Fix Adjust APIs #3753

charliegracie · 2019-04-17T16:40:57Z

JitBuilder math APIs like Add, Mul, etc. require that the left and
right operands have the same type or if the left is a pointer that
the right is an Int32 or Int64. Currently a VMRegister wrapping a
int8_t * will fail when Adjust is called since
_integerTypeForAdjustment will be Int8. It is incorrect to limit
the adjustBy amount to the primite type of the pointer since a user
may want to adjust the value by a large value. The solution is to
adjust by an Int64 not matter what the primitive register type is.

This PR includes a new test called vmregister which verifies that
VMRegisters wrapping a pInt8 work. This test failed before this
PR. The Operand[Array,Stack]Tests verify that the code continues
to work for larger types.

Signed-off-by: Charlie Gracie charlie.gracie@gmail.com

charliegracie · 2019-04-17T18:51:51Z

@genie-omr build all

charliegracie · 2019-04-22T19:53:05Z

FYI @Leonardo2718 and @mstoodle

charliegracie · 2019-04-29T17:55:15Z

@Leonardo2718 any thoughts?

mstoodle · 2019-04-29T20:33:18Z

Shouldn't the right thing to be adjust by a Word sized integer? Int64 would only be the right choice on 64-bit platforms...

charliegracie · 2019-04-30T12:11:52Z

@mstoodle the virtual void Adjust(TR::IlBuilder *b, int64_t amount) has an int64_t parameter. To match the API signature and make sure that values are not truncated I believe the code needs to use ConstInt64.

mstoodle · 2019-04-30T13:50:16Z

We could change that to a size_t or assert if a value that can't be represented in 32-bits it provided on a 32-bit platform.

The resulting IL will be ill-formed if you add a 64-bit value to a 32-bit address.

Leonardo2718 · 2019-04-30T15:26:38Z

The resulting IL will be ill-formed if you add a 64-bit value to a 32-bit address.

True.

charliegracie · 2019-05-02T13:53:38Z

@genie-omr build all

charliegracie · 2019-05-02T14:24:11Z

@genie-omr build all

charliegracie · 2019-05-02T14:30:52Z

@genie-omr build xlinux

charliegracie · 2019-05-02T15:35:32Z

@mstoodle I updated the code. Can you take another look?

charliegracie · 2019-05-06T17:23:08Z

ping?

charliegracie · 2019-05-14T19:20:07Z

ping @mstoodle

mstoodle · 2019-05-14T20:47:44Z

Sorry for the delay here, @charliegracie . Every time I start reviewing the changes in OMRVirtualMachineRegister I get stuck wondering about the real point of this code. The cascade of if statements looking progressively "deeper" into pointer types strike me as over complicated for what VirtualMachineRegister is supposed to be. Then I inevitably get distracted without circling back to a conclusion. Not fair to you; sorry!

At its core level, a VirtualMachineRegister was supposed to represent some kind of "state" variable stored at some address. The "at some address" part is where the "double" PointerTo() expectation comes from in the common case of a state register that points somewhere (like a PC or an SP): OMRVirtualMachineRegister needs to store the address of the actual state variable so that it can store to that address indirectly when Commit() is called.

In our private exchange, you pointed out that it's through OMRVirtualMachineRegisterInStruct that the problem actually appears, and I think that's what is causing the root problem here: in the constructor for OMRVirtualMachineRegisterInStruct, it uses the same "double pointer type" decoding logic as in OMRVMReg and I think that's wrong. For OMRVirtualMachineRegisterInStruct, I think it should be setting _integerTypeForAdjustments Word for any field that is a pointer, and to the field type for anything else. I believe that should resolve your core problem and is the most correct fix.

Sorry again for the delay to fully wrap my brain around the issue and the back-and-forth on the proper solution!

mstoodle

Hopefully straight-forward changes....Thanks @charliegracie!

mstoodle · 2019-05-15T20:10:29Z

compiler/ilgen/OMRVirtualMachineRegister.cpp

-   _integerTypeForAdjustments = pointerToRegisterType->baseType();
-   if (_integerTypeForAdjustments->isPointer())
+   TR::IlType *registerBaseType = pointerToRegisterType->baseType();
+   if (registerBaseType->isPointer())


I think these conditionals can stay the way they are (keeping _integerTypeForAdjustments). I prefer the register know what type to use based on the type of the register rather than relying on the datatype of the adjustment value passed in.

mstoodle · 2019-05-15T20:11:10Z

compiler/ilgen/OMRVirtualMachineRegister.hpp

@@ -161,7 +161,8 @@ class VirtualMachineRegister : public TR::VirtualMachineState
   virtual void Adjust(TR::IlBuilder *b, TR::IlValue *amount)
      {
      TR::IlValue *off=b->Mul(amount,
-                       b->    ConstInteger(_integerTypeForAdjustments, _adjustByStep));
+                       b->    ConstInteger(b->typeDictionary()->PrimitiveType(amount->getDataType()),


This is more dangerous, I think, because it requires the client to set up the type properly for every call to this Adjust service.

Probably best to add a b->ConvertTo(_integerTypeForAdjustments, amount) here too rather than just using amount directly.

mstoodle · 2019-05-15T20:14:13Z

compiler/ilgen/OMRVirtualMachineRegisterInStruct.hpp

-         _integerTypeForAdjustments = _integerTypeForAdjustments->baseType();
-         if (_integerTypeForAdjustments->isPointer())
+         registerBaseType = registerBaseType->baseType();
+         if (registerBaseType->isPointer())


I don't think this should dig into two levels of pointer here: it starts from the field type, which should only require one level of pointer to primitive type to do the right thing (i.e. if it's a pointer type use Word otherwise use the field's primitive type).

It's possible this change may induce some required changes in our samples, but I would hope not.

charliegracie · 2019-05-22T01:27:50Z

@mstoodle I made most of the changes you suggested but now my test is failing. I have purposely left out the ConvertTo call you suggested as it would not help for the failure I am seeing. When I run the new vmregister test (which does not use VirtualMachineRegisterInStruct) I get the following assertion which is being generated because of the Adjust call:

Step 1: initialize JIT
Step 2: define type dictionary
Step 3: compile method builder
Assertion failed at /Users/charliegracie/vs_workspaces/omr/compiler/ilgen/OMRIlBuilder.cpp:1116: leftType == rightType || isAddressBump || isRevAddressBump
	binaryOp requires both left and right operands to have same type or one is address and other is Int32/64
compiling /Users/charliegracie/vs_workspaces/omr/jitbuilder/release/cpp/samples/VMRegister.cpp:81:test at level: warm

.......
frame #7: 0x0000000000148e94 vmregister`OMR::IlBuilder::binaryOpNodeFromNodes(TR::ILOpCodes, TR::Node*, TR::Node*) + 340
    frame #8: 0x0000000000148f1d vmregister`OMR::IlBuilder::binaryOpFromNodes(TR::ILOpCodes, TR::Node*, TR::Node*) + 61
    frame #9: 0x00000000001472ca vmregister`OMR::IlBuilder::Add(TR::IlValue*, TR::IlValue*) + 378
    frame #10: 0x0000000000174791 vmregister`OMR::VirtualMachineRegister::adjust(TR::IlBuilder*, TR::IlValue*) + 145
    frame #11: 0x000000000001a166 vmregister`OMR::VirtualMachineRegister::Adjust(TR::IlBuilder*, long long) + 86
    frame #12: 0x0000000000019bff vmregister`OMR::JitBuilder::VirtualMachineRegister::Adjust(OMR::JitBuilder::IlBuilder*, unsigned long) + 111
    frame #13: 0x00000000000033fc vmregister`VMRegisterMethod::buildIL() + 684
.....

mstoodle · 2019-05-22T06:04:12Z

I think OMRVirtualMachineRegister is digging too deeply into its pointerToRegister type too :( .

To have adjust() work with the Word type, right now you have to provide a triple pointer, which doesn't make sense to me: you're adjusting the value that's one level of indirection away (i.e. you provided the address of the register you're modelling and I don't think you should be touching any values pointed at by the register). I am not enough of a git wizard to track back why that change was introduced, but the code right now does not make sense to me.

These VirtualMachineRegister classes should be looking at as many as 2 levels of indirection (pointer types). If the provided type is pointer to pointer to any type, then the adjustment type should be Word because when you adjust the register value you're adjusting a pointer. If the type is pointer to non-pointer integer type, then similarly the adjustment type should be the non-pointer integer type. For any type that doesn't fit those two patterns, one probably shouldn't be using adjust but rather rolling your own because I'm not sure we could predict what is for certain the "right" thing to do. Well, I suppose we could add float/double variants of adjust but not sure they would be used much.

I am, however, fairly certain something will break if we change the _integerTypeForAdjustments logic in OMR::VirtualMachineRegister's constructor (maybe in T4: @jduimovich or Lua: @Leonardo2718 ... Any of this ring bells for you guys? )

charliegracie · 2019-05-22T16:01:10Z

@genie-omr build all

charliegracie · 2019-05-22T16:05:45Z

@mstoodle I think I got it :) If all of the tests pass I think I wrapped my head around everything and have the correct changes now

mstoodle · 2019-05-28T01:12:29Z

compiler/ilgen/OMRIlBuilder.hpp

-   TR::IlValue *Const(int32_t value)            { return ConstInt32(value); }
-   TR::IlValue *Const(int64_t value)            { return ConstInt64(value); }
+
+   template <typename T>


Would you consider moving this change into a separate commit, since it doesn't contribute to the main change you're making at this point?

A separate commit in this PR or a new PR?

@mstoodle I will just pull it out of this PR completely

sorry, missed the question....either would have been fine, but since it's not related to the PR, feels like a separate PR is most appropriate.

mstoodle · 2019-05-28T01:13:09Z

Basically looks good to me at this point. @Leonardo2718 do you want to do a once over to see if anything still stands out to you?

Leonardo2718

LGTM 👍

JitBuilder math APIs like Add, Mul, etc. require that the left and right operands have the same type or if the left is a pointer that the right is an Int32 or Int64. Currently a VMRegister wrapping a int8_t * will fail when Adjust is called since _integerTypeForAdjustment will be Int8. It is incorrect to limit the adjustBy amount to the primitive type of the pointer since a user may want to adjust the value by a large value. The Adjust API now takes a `size_t` amount parameter and creates the correct constant for adjustment. This PR includes a new test called vmregister which verifies that VMRegisters wrapping a pInt8 work. This test failed before this PR. The Operand[Array,Stack]Tests verify that the code continues to work for larger types. Signed-off-by: Charlie Gracie <charlie.gracie@gmail.com>

charliegracie · 2019-05-30T13:59:51Z

@genie-omr build all

charliegracie · 2019-06-03T12:22:04Z

@mstoodle and @Leonardo2718 I have removed the new APIs that were no longer used.

mstoodle · 2019-06-03T14:18:51Z

LGTM , merging. Thanks @charliegracie !!

charliegracie added the comp:jitbuilder label Apr 17, 2019

charliegracie requested review from Leonardo2718 and mstoodle as code owners April 17, 2019 16:40

charliegracie force-pushed the vmregister branch from e70e788 to 02678cf Compare April 17, 2019 16:42

charliegracie force-pushed the vmregister branch from 02678cf to 0fa8e55 Compare May 2, 2019 13:53

charliegracie force-pushed the vmregister branch from 0fa8e55 to 529efaa Compare May 2, 2019 14:23

mstoodle self-assigned this May 15, 2019

mstoodle requested changes May 15, 2019

View reviewed changes

charliegracie force-pushed the vmregister branch 2 times, most recently from 946ee39 to 9ab435e Compare May 22, 2019 01:24

charliegracie force-pushed the vmregister branch from 9ab435e to 0a9023e Compare May 22, 2019 15:58

mstoodle approved these changes May 28, 2019

View reviewed changes

Leonardo2718 approved these changes May 28, 2019

View reviewed changes

charliegracie force-pushed the vmregister branch from 0a9023e to da1e49d Compare May 30, 2019 13:59

mstoodle merged commit f1a0533 into eclipse-omr:master Jun 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Adjust APIs #3753

Fix Adjust APIs #3753

charliegracie commented Apr 17, 2019

charliegracie commented Apr 17, 2019

charliegracie commented Apr 22, 2019

charliegracie commented Apr 29, 2019

mstoodle commented Apr 29, 2019

charliegracie commented Apr 30, 2019

mstoodle commented Apr 30, 2019

Leonardo2718 commented Apr 30, 2019

charliegracie commented May 2, 2019

charliegracie commented May 2, 2019

charliegracie commented May 2, 2019

charliegracie commented May 2, 2019

charliegracie commented May 6, 2019

charliegracie commented May 14, 2019

mstoodle commented May 14, 2019 •

edited

Loading

mstoodle left a comment

mstoodle May 15, 2019 •

edited

Loading

mstoodle May 15, 2019

mstoodle May 15, 2019 •

edited

Loading

mstoodle May 15, 2019

mstoodle May 15, 2019

charliegracie commented May 22, 2019

mstoodle commented May 22, 2019

charliegracie commented May 22, 2019

charliegracie commented May 22, 2019

mstoodle May 28, 2019 •

edited

Loading

charliegracie May 28, 2019

charliegracie May 30, 2019

mstoodle May 30, 2019

mstoodle commented May 28, 2019

Leonardo2718 left a comment

charliegracie commented May 30, 2019

charliegracie commented Jun 3, 2019

mstoodle commented Jun 3, 2019

Fix Adjust APIs #3753

Fix Adjust APIs #3753

Conversation

charliegracie commented Apr 17, 2019

charliegracie commented Apr 17, 2019

charliegracie commented Apr 22, 2019

charliegracie commented Apr 29, 2019

mstoodle commented Apr 29, 2019

charliegracie commented Apr 30, 2019

mstoodle commented Apr 30, 2019

Leonardo2718 commented Apr 30, 2019

charliegracie commented May 2, 2019

charliegracie commented May 2, 2019

charliegracie commented May 2, 2019

charliegracie commented May 2, 2019

charliegracie commented May 6, 2019

charliegracie commented May 14, 2019

mstoodle commented May 14, 2019 • edited Loading

mstoodle left a comment

Choose a reason for hiding this comment

mstoodle May 15, 2019 • edited Loading

Choose a reason for hiding this comment

mstoodle May 15, 2019

Choose a reason for hiding this comment

mstoodle May 15, 2019 • edited Loading

Choose a reason for hiding this comment

mstoodle May 15, 2019

Choose a reason for hiding this comment

mstoodle May 15, 2019

Choose a reason for hiding this comment

charliegracie commented May 22, 2019

mstoodle commented May 22, 2019

charliegracie commented May 22, 2019

charliegracie commented May 22, 2019

mstoodle May 28, 2019 • edited Loading

Choose a reason for hiding this comment

charliegracie May 28, 2019

Choose a reason for hiding this comment

charliegracie May 30, 2019

Choose a reason for hiding this comment

mstoodle May 30, 2019

Choose a reason for hiding this comment

mstoodle commented May 28, 2019

Leonardo2718 left a comment

Choose a reason for hiding this comment

charliegracie commented May 30, 2019

charliegracie commented Jun 3, 2019

mstoodle commented Jun 3, 2019

mstoodle commented May 14, 2019 •

edited

Loading

mstoodle May 15, 2019 •

edited

Loading

mstoodle May 15, 2019 •

edited

Loading

mstoodle May 28, 2019 •

edited

Loading