Closed rbanerjee closed 6 years ago
I have seen some flavor of these failures at some point. I could not reproduce however with the GCC version I was using when these tests were checked in.
In any event these failures are almost certainly related to the ABI and the code generators not explicitly supporting sub 32-bit integer arguments. The ABI on Linux on z will widen the arguments to 32-bit before passing the argument through the register. This needs to be more closely investigated. I'll put this on our backlog and we will investigate this issue.
Wanted to double check.. in the snprintf
, are one of the parms suppose to be param.val1
?
std::snprintf(inputTrees, sizeof(inputTrees),
...
param.val2, // value of 'bconst' in 'band'
param.val2 // value of 'bconst' in 'ifbucmpge'
Wanted to double check.. in the snprintf, are one of the parms suppose to be param.val1?
@joransiu no this is as intended. If you look at the oracle, the constant you are ANDing with and the constant you're comparing for greater-than-or-equal is the same, i.e. param.val2
:
We don't use param.val1
in the std::snprintf(inputTrees
because the lefthand side of the AND operation comes in through an argument to the JIT method, i.e.
@fjeremic got it. Thanks for the clarification. Would be interesting to see the generated sequence for that IL tree.
@joransiu Here are the trace logs for a smaller test case. (In case you don't have everything setup.)
Before I modified the Test, GetParam() = ('\x1' (1), '\x1' (1), 0x803f6ee0)
used to pass but after my modification it fails with the following output (compiled with GCC 4.4.6
) -
Note: Google Test filter = IfxcmpgeReductionTest/UInt8ReductionTest.Reduction/10
Failure
Expected: param.oracle(param.val1, param.val2)
Which is: 1
To be equal to: entry_point(param.val1)
Which is: 0
[ FAILED ] IfxcmpgeReductionTest/UInt8ReductionTest.Reduction/10, where GetParam() = ('\x1' (1), '\x1' (1), 0x803f6ee0) (7 ms)
[----------] 1 test from IfxcmpgeReductionTest/UInt8ReductionTest (8 ms total)
[==========] 1 test from 1 test case ran. (10 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] IfxcmpgeReductionTest/UInt8ReductionTest.Reduction/10, where GetParam() = ('\x1' (1), '\x1' (1), 0x803f6ee0)
As you can see, the oracle is clearly correct, but the modified version (the one where I "fix" the method signature and replace the iload / i2b
with a bload
) does something wrong.
Before my modification: log_IfxcmpgeUInt8_10_tracefull_pre.txt
(generated via: TR_Options='tracefull,log=log_IfxcmpgeUInt8_10_tracefull_pre' ./fvtest/compilertriltest/comptest --gtest_filter=IfxcmpgeReductionTest/UInt8ReductionTest.Reduction/10
)
After making the changes: log_IfxcmpgeUInt8_10_tracefull.txt
Also, here's one where the original version fails:
GetParam() = ('\0', '\xFF' (255), 0x803f6ee0)
Expected: param.oracle(param.val1, param.val2)
Which is: 0
To be equal to: entry_point(param.val1)
Which is: 1
[ FAILED ] IfxcmpgeReductionTest/UInt8ReductionTest.Reduction/2, where GetParam() = ('\0', '\xFF' (255), 0x803f6ee0) (7 ms)
[----------] 1 test from IfxcmpgeReductionTest/UInt8ReductionTest (7 ms total)
[==========] 1 test from 1 test case ran. (8 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] IfxcmpgeReductionTest/UInt8ReductionTest.Reduction/2, where GetParam() = ('\0', '\xFF' (255), 0x803f6ee0)
trace_log: log_IfxcmpgeUInt8_2_tracefull_pre.txt
The ABI extends sub 32-bit integer to full 32-bit integer values. After changing the iload/i2b
sequence to bload
, we end up loading only a single byte from the value of the stack slot.
0x3fffbfd603c 0000000c [ 0xb27a6200] 50 20 f0 b0 ST GPR2,#585 176(GPR15)
...
0x3fffbfd6040 00000010 [ 0xb27a27d0] 91 01 f0 b0 TM Parm[Parm 0<parm 0 B>] 176(GPR15), 0x01
0x3fffbfd6044 00000014 [ 0xb27a28a0] a7 14 00 8a BRC BP(0x1), Label L0016, labelTargetAddr=0x0x3fffbfd6056
We ended up generating the TM 176(R15), 0x1
for the bload
, which is reading the most significant byte of the 32-bit integer parameter (given we are big endian).
We ended up generating the TM 176(R15), 0x1 for the bload, which is reading the most significant byte of the 32-bit integer parameter (given we are big endian).
@joransiu @fjeremic Curious, is this expected? I had the idea that the IL was supposed to be "architecture agnostic". As in if I'm doing a i2b
~b2i
~ on a TR::Int32
, regardless of whether I'm on a big endian or a little endian machine, I should be getting the same byte.
Also, should calling bload
on a Int32
param be legal (I don't think it should)? Assuming it is, shouldn't that behaviour be "architecture agnostic" as well?
Curious, is this expected?
I guess that's up for debate and there is already a huge discussion of it in #1901. The ABI determines how arguments get passed. For Linux on z the ABI dictates that arguments come in with at least 32-bits wide and this is why the parameter is defined as Int32
. JitBuilder seems to store these arguments to the stack (using a 32-bit stack slot). So it is incorrect to reference the same parameter as an Int8
later on.
Also, should calling bload on a Int32 param be legal (I don't think it should)? Assuming it is, shouldn't that behaviour be "architecture agnostic" as well?
Again this is up for debate (see #1901). As things currently stand in the code generator, no this should not be legal. There needs to be conversion trees inserted if you really want the least significant byte of a 32-bit integer.
Investigation so far:
I've looked at the following failure in isolation:
IfxcmpgeReductionTest/UInt8ReductionTest.Reduction/2, where GetParam() = ('\0', '\xFF' (255), 0x803f6ee0)
The input parameters are of type uint8_t
. The first parameter, a
, is 0 and the second parameter, b, is 0xFF (or 255 since it's unsigned).
The oracle function (used to verify the test results) is essentially:
return (a & b) >= b
So if a = 0, b = 255, then the oracle should return 0 (or false). And this is exactly what happens. So the oracle is correct.
The problem occurs in the way the IL is written. We specify the value 0xFF
via a bconst
node, which is a signed type. So the jit compiler sees 0xFF as -1. When b is -1, the oracle will actually return 1 (since 0 is greater than -1).
From my understanding, unsigned nodes in our IL are deprecated. So I don't see the point of these unsigned tests. We should remove these unsigned tests altogether. The tree currently looks like the following:
109 " (block name=\"b\" fallthrough=\"f\""
110 " (ifbucmpge target=\"t\""
111 " (band"
112 " (i2b"
113 " (iload parm=0) )"
114 " (bconst %d) )"
115 " (bconst %d) ) )"
116 " (block name=\"f\""
117 " (ireturn (iconst 0) ) )"
118 " (block name=\"t\""
119 " (ireturn (iconst 1) ) ) )",
Ideally the bconst %d
on line 114 and line 115 need to be changed to use buconst
instead. However since these nodes are deprecated, we can't. I don't see how we could ever end up with an ifbucmpge
node where the second child is a bconst
node. In fact, I'm not sure if the i2b
node in the tree above is correct as well (considering we are dealing with unsigned types).
If however there is a way that the above tree is possible, then I can fix these tests by modifying the oracle so that it recognizes the type of the second parameter correctly. The second parameter will always need to be a signed integer because the IL also reads them as signed integers. Right now it assume both parameters have the same type.
I suggest we remove the unsigned tests since we are doing a signed AND under a root that has the unsigned type property(doesn't make sense).
Thoughts?
For reference, here's the binary encoding for the case mentioned above:
1596 <instructions
1597 title="Post Binary Instructions"
1598 method="file:line:name"
1599 hotness="warm">
1600
1601 +--------------------------------------- instruction address
1602 | +----------------------------------------- instruction offset from start of method
1603 | | +------------------------------------------ corresponding TR::Instruction instance
1604 | | | +-------------------------------------------------- code bytes
1605 | | | | +-------------------------------------- opcode and operands
1606 | | | | | +----------- additional information
1607 | | | | | |
1608 V V V V V V
1609 0x3fffc109030 00000000 [ 0xb7f65df0] PROC
1610 0x3fffc109030 00000000 [ 0xb7f69b60] e3 f0 f0 78 00 24 STG GPR15,#582 120(GPR15)
1611 0x3fffc109036 00000006 [ 0xb7f69d00] e3 f0 ff 60 ff 71 LAY GPR15,#583 -160(GPR15)
1612 0x3fffc10903c 0000000c [ 0xb7f69f90] 50 20 f0 b0 ST GPR2,#585 176(GPR15)
1613 0x3fffc109040 00000010 [ 0xb7f65f10] FENCE Relative [ 0xb7ed1e10 ] BBStart <block_2> (frequency 10000)
1614 0x3fffc109040 00000010 [ 0xb7f66390] 58 00 f0 b0 L GPR0, Parm[Parm 0<parm 0 I>] 176(GPR15)
1615 0x3fffc109044 00000014 [ 0xb7f66460] a7 0e ff ff CHI GPR0,0xffff
1616 0x3fffc109048 00000018 [ 0xb7f66530] a7 a4 00 8a BRC BNM(0xa), Label L0016, labelTargetAddr=0x0x3fffc10905a
1617 0x3fffc10904c 0000001c [ 0xb7f66850] FENCE Relative [ 0xb7ed1e14 ] BBEnd </block_2>
1618 0x3fffc10904c 0000001c [ 0xb7f669d0] FENCE Relative [ 0xb7ed1f50 ] BBStart <block_3> (frequency 8001) (extension of previous block)
1619 0x3fffc10904c 0000001c [ 0xb7f66c80] 17 22 XR GPR2,GPR2
1620 0x3fffc10904e 0000001e [ 0xb7f66da0] b9 14 00 22 LGFR GPR2,GPR2
1621 0x3fffc109052 00000022 [ 0xb7f670a0] ASSOCREGS
1622 0x3fffc109052 00000022 [ 0xb7f6a0c0] Label L0032:
1623 0x3fffc109052 00000022 [ 0xb7f6a330] e3 f0 f1 18 00 04 LG GPR15,#587 280(GPR15)
1624 0x3fffc109058 00000028 [ 0xb7f6a400] 07 fe BCR BER(mask=0xf), GPR14
1625 0x3fffc10905a 0000002a [ 0xb7f66e70] RET
1626 0x3fffc10905a 0000002a [ 0xb7f672d0] FENCE Relative [ 0xb7ed1f54 ] BBEnd </block_3>
1627 0x3fffc10905a 0000002a [ 0xb7f67500] ASSOCREGS
1628 0x3fffc10905a 0000002a [ 0xb7f67680] Label L0016:
1629 0x3fffc10905a 0000002a [ 0xb7f67750] FENCE Relative [ 0xb7ed2090 ] BBStart <block_4> (frequency 8001)
1630 0x3fffc10905a 0000002a [ 0xb7f67a30] a7 28 00 01 LHI GPR2,0x1
1631 0x3fffc10905e 0000002e [ 0xb7f67b50] b9 14 00 22 LGFR GPR2,GPR2
1632 0x3fffc109062 00000032 [ 0xb7f67e50] ASSOCREGS
1633 0x3fffc109062 00000032 [ 0xb7f6a520] Label L0033:
1634 0x3fffc109062 00000032 [ 0xb7f6a790] e3 f0 f1 18 00 04 LG GPR15,#589 280(GPR15)
1635 0x3fffc109068 00000038 [ 0xb7f6a860] 07 fe BCR BER(mask=0xf), GPR14
1636 0x3fffc10906a 0000003a [ 0xb7f67c20] RET
1637 0x3fffc10906a 0000003a [ 0xb7f68080] FENCE Relative [ 0xb7ed2094 ] BBEnd </block_4>
1638 0x3fffc10906a 0000003a [ 0xb7f682b0] ASSOCREGS
Also, pre-instruction selection trees:
1048 title="Pre Instruction Selection Trees"
1049 method="file:line:name"
1050 hotness="warm">
1051
1052 Pre Instruction Selection Trees: for file:line:name
1053
1054 ------------------------------------------------------------------------------------------------------------------------------------------------------------------------
1055 n1n BBStart <block_2> (freq 10000) [ 0x96278c70] bci=[-1,0,-] rc=0 vc=61 vn=- li=2 udi=- nc=0
1056 n8n ifbucmpge --> block_4 BBStart at n5n () [ 0x96278e30] bci=[-1,0,-] rc=0 vc=61 vn=- li=2 udi=- nc=2 flg=0x20
1057 n10n i2b [ 0x96278eb0] bci=[-1,0,-] rc=1 vc=61 vn=- li=2 udi=- nc=1
1058 n11n iload Parm 0<parm 0 I>[#581 Parm] [flags 0xc0000103 0x0 ] [ 0x96278ef0] bci=[-1,0,-] rc=1 vc=61 vn=- li=2 udi=- nc=0
1059 n13n bconst -1 [ 0x96278f70] bci=[-1,0,-] rc=1 vc=61 vn=- li=2 udi=- nc=0
1060 n2n BBEnd </block_2> [ 0x96278cb0] bci=[-1,0,-] rc=0 vc=61 vn=- li=2 udi=- nc=0
1061 n3n BBStart <block_3> (freq 8001) (extension of previous block) [ 0x96278cf0] bci=[-1,0,-] rc=0 vc=61 vn=- li=3 udi=- nc=0
1062 n14n ireturn [ 0x96278fb0] bci=[-1,0,-] rc=0 vc=61 vn=- li=3 udi=- nc=1
1063 n15n iconst 0 [ 0x96278ff0] bci=[-1,0,-] rc=1 vc=61 vn=- li=3 udi=- nc=0
1064 n4n BBEnd </block_3> ===== [ 0x96278d30] bci=[-1,0,-] rc=0 vc=61 vn=- li=3 udi=- nc=0
1065
1066 n5n BBStart <block_4> (freq 8001) [ 0x96278d70] bci=[-1,0,-] rc=0 vc=61 vn=- li=4 udi=- nc=0
1067 n16n ireturn [ 0x96279030] bci=[-1,0,-] rc=0 vc=61 vn=- li=4 udi=- nc=1
1068 n17n iconst 1 [ 0x96279070] bci=[-1,0,-] rc=1 vc=61 vn=- li=4 udi=- nc=0
1069 n6n BBEnd </block_4> [ 0x96278db0] bci=[-1,0,-] rc=0 vc=61 vn=- li=4 udi=- nc=0
If however there is a way that the above tree is possible, then I can fix these tests by modifying the oracle so that it recognizes the type of the second parameter correctly. The second parameter will always need to be a signed integer because the IL also reads them as signed integers. Right now it assume both parameters have the same type.
I suggest we remove the unsigned tests since we are doing a signed AND under a root that has the unsigned type property(doesn't make sense).
This is because when doing comparisons we need to have the notion of signed / unsigned comparisons, otherwise you cannot handle the expression a >= b
when both types are unsigned. We need to know at codegen time whether to generate an arithmetic (signed) or logical (unsigned) comparison.
i.e. should we generate CHI
(arithmetic signed comparison) or CLFHSI
(logical unsigned comparison)? They will yield different results. In the above case we generated CHI
implying we thought we were doing a signed comparison when in fact what we wanted was an unsigned comparison. It seems the ifbucmpeq
evaluator did not honor this (we should have generated CLFHSI
).
IMO this is why we need a notion of signed and unsigned comparisons. How would we handle this if we are deprecating all unsigned IL? @mgaudet @joransiu @mstoodle @ymanton thoughts? Is this really what we are doing? Or are we only deprecating unsigned IL where the sign of the operands don't really matter, for example an AND operation, or a *const
IL.
@dchopra001 it may be a worthy exercise here to just hop in gdb
or whatever debugger you like and verify the content of the argument register in the JIT method. Is it signed or logically sign extended? The ABI will dictate this.
http://legacy.redhat.com/pub/redhat/linux/7.1/es/os/s390x/doc/lzsabi0.pdf Page 14 says:
Values shorter than 64-bits are sign- or zero-extended (as appropriate) to 64 bits.
Can you post what the content of the argument (GPR2
) register is upon entry to this JIT body?
IMO this is why we need a notion of signed and unsigned comparisons. How would we handle this if we are deprecating all unsigned IL? @mgaudet @joransiu @mstoodle @ymanton thoughts? Is this really what we are doing? Or are we only deprecating unsigned IL where the sign of the operands don't really matter, for example an AND operation, or a *const IL.
I think we're only talking about getting rid of unsigned variants of opcodes where the computation is sign-bit insensitive. i.e, iuload
is out, but iu2l
is in.
Can you post what the content of the argument (GPR2) register is upon entry to this JIT body?
I hopped into GDB to verify the values. GPR2 should have 0 in it because we pass the first parameter into the function (which is 0). 255 is a const that the method specifies. The value in GPR2 is indeed 0, so it is behaving correctly. So as you suggested earlier, the problem is most likely due to this: CHI GPR0,0xffff
. It looks like the compiler also sign extended the 0xff to 0xffff. I'll look in the ifbucmpgt
for further clues.
@fjeremic we do have signed/unsigned comparisons :) . Even JitBuilder has signed/unsigned comparisons :) .
The direction has been that data is not signed or unsigned: it's just a bit pattern. Most processors, for example, do not have signed/unsigned registers but some instructions have signed/unsigned variants. The IL is based on and has been influenced for some time now towards the same philosophy. @ofrobots did the work a number of years ago to make our IL more consistent in this regard (thanks, Ali :) ).
Some operations are not signed or unsigned (like your AND example). Other operations act differently for signed/unsigned (like signed compare less than / unsigned compare less than).
Update on investigation:
It looks like the issue is in one of the helpers for the ifbucmpgt
evaluators. Before generating the compare instruction, we specify an incorrect opCode for 8 and 16bit data types here:
https://github.com/eclipse/omr/blob/ef7746b30ef9b951488c2d237fa98c2010a3b5e9/compiler/z/codegen/OMRTreeEvaluator.cpp#L3380-L3384
I'm not sure why we don't pick the right op for the TR::Int8
and TR::Int16
cases but do it correctly for the case when dataType isTR::Int32
:
https://github.com/eclipse/omr/blob/ef7746b30ef9b951488c2d237fa98c2010a3b5e9/compiler/z/codegen/OMRTreeEvaluator.cpp#L3375-L3378
This also explains why the failures are only seen on UInt8 and UInt16 tests.
Here's a proposed fix: https://github.com/dchopra001/omr/commit/87080a5a52efab5f9585f242bc38c2e42bc478e5
All tril tests pass with this fix.
I'll open a PR for this.
Also, on a side note I see we used getIntegralValue()
in the above example, but in the case where the DataType is TR::Int32 or TR::Int64
. Is there any particular reason for the inconsistency? I don't really see the value of using the getIntegralValue
helper in this situation anyway.
The function might also need to be updated if unsigned data nodes are deprecated: https://github.com/eclipse/omr/blob/ef7746b30ef9b951488c2d237fa98c2010a3b5e9/compiler/z/codegen/OMRTreeEvaluator.cpp#L476-L478
The Tril based compiler tests for
IfxcmpgeReduction
fail on zlinux_s390x (built usingGCC 4.4.6
)I carried out an initial investigation, and the associated trees look wrong to me.
For example these are tests failing for unsigned int8_t:
with the test case being:
Note that the Tril method signature and the compiler entry point signature (
compiler.getEntryPoint<int32_t (*)(int8_t)>()
) disagree. Also, it's trying to load a byte with aniconst
.After "fixing" these I get the following testcase -
running that gives me this:
Which is actually more than what I started off with : ] But are still different from the ones I was getting before.
So not really sure whether loading the byte via an iload was done intentionally. Maybe @fjeremic could chip in here, I think these were added by you in Issue #1557 and I'm not sure if this is something
Z
specific (since the tests seem to pass on x86 but then again that doesn't really guarantee their correctness).