Question on arithmetic overflow detection support in C2

Krystal Mok rednaxelafx at gmail.com
Wed Jun 20 18:29:25 PDT 2012


I traced the compilation, and found that CheckedAddINode::match() was never
called before hitting the assert.
The available rules for Proj #1 (the conditions projection) doesn't look
right in the output log, does it?

- Kris

On Thu, Jun 21, 2012 at 8:28 AM, Krystal Mok <rednaxelafx at gmail.com> wrote:

> On Thu, Jun 21, 2012 at 8:03 AM, Vladimir Kozlov <
> vladimir.kozlov at oracle.com> wrote:
>
>> Krystal Mok wrote:
>>
>>> Thanks for the reply! Comments inline below:
>>>
>>> On 2012-6-21, at 6:13, Vladimir Kozlov <vladimir.kozlov at oracle.com>
>>> wrote:
>>>
>>>  Look on output before assert message. It should have problematic ideal
>>>> subgraph dump which it can't match.
>>>>
>>>>  The output is:
>>>
>>
>> You did not send output.
>>
>> Oops, I hit the send button too early ...
>
> Here's the output:
>
> o30 If === o5 o46 o45  [[o31 o32  6 ]] P=0.000001, C=-1.000000
>
> --N: o30 If === o5 o46 o45  [[o31 o32  6 ]] P=0.000001, C=-1.000000
>
>    --N: o46 Bool === _ o45  [[o30 ]] [??]
>    CMPOP  0  CMPOP
>    CMPOPU  0  CMPOPU
>
>    --N: o45 Proj === o43  [[o46 o30 ]] #1
>    RREGI  0  RREGI
>    RAX_REGI  0  RAX_REGI
>    RBX_REGI  0  RBX_REGI
>    RCX_REGI  0  RCX_REGI
>    RDX_REGI  0  RDX_REGI
>    RDI_REGI  0  RDI_REGI
>    NO_RCX_REGI  0  NO_RCX_REGI
>    NO_RAX_RDX_REGI  0  NO_RAX_RDX_REGI
>    STACKSLOTI  100  storeSSI
>
>
>
>>
>>
>>>  I think the problem is that you have Proj node in between If and
>>>> CheckedAddI nodes so that it can't match 'If' node since it looks for
>>>> inputs which produce flags (cr). May be CheckedAddINode::match() is not
>>>> enough.
>>>>
>>>>  You're right, that's exactly what's happening.
>>>
>>
> One more question on this one: how does the matcher know that the data
> projection should be in the same location as the dst input? There's nowhere
> to specify it.
>
>
>>
>>>  Also you can't use EMPTY mask for data result projection. Bind it to an
>>>> register as DivMod node.
>>>>
>>>>
>> Mask should reflect in what registers could be result. You don't need to
>> bind to specific register but you need to return non-empty mask, for
>> example, INT_REG_mask(). And for flag projection you need INT_FLAGS_mask().
>
>
> Ahh, so the register class declaration for RFLAGS moved to x86.ad ... no
> wonder I couldn't find it when I searched x86_64.ad.
>
> I'm still getting the same crash after I changed CheckedAddINode::match()
> to this:
>
> Node* CheckedAddINode::match(const ProjNode* proj, const Matcher* match) {
>   uint ideal_reg = proj->ideal_reg();
>   RegMask rm = RegMask::Empty;
>   if (proj->_con == sum_proj_num) {
>     rm = match->checkedAddI_sum_proj_mask();
>   } else {
>     assert(proj->_con == overflow_proj_num, "must be sum or overflow
> projection");
>     ideal_reg = Op_RegFlags;
>     rm = match->overflow_proj_mask();
>   }
>   return new (match->C, 1) MachProjNode(this, proj->_con, rm, ideal_reg);
> }
>
> Where both RegMasks specified as suggested above. The output log is still
> the same.
>
> Thanks,
> Kris
>
>
>>
>>
>> Vladimir
>>
>>
>>  But in the DivMod case, both data projections had to be in fixed
>>> registers because that's required by idiv, where as here binding the sum
>>> projection to a fixed register (e.g. rax) would be less optimal, right?
>>>
>>> If only there's a way to express the (Set dest ...) semantic for
>>> CheckedAddI's data projection ...
>>>
>>> Anyway, binding it fixed should at least get the example running. I'll
>>> try this first and look for alternative solutions later.
>>>
>>>  And use masm instructions encoding
>>>>
>>>>  ins_encode %{
>>>>   __ addl($dst$$Register, $src$$Register);
>>>>  %}
>>>>
>>>> instead of
>>>>
>>>>  ins_encode(REX_reg_reg(dst, src), OpcP, reg_reg(dst, src));
>>>>
>>>>  Will fix this one. I just copied the code from the basic instruct for
>>> AddI, which is still using the old ins_encode form.
>>>
>>> Thanks,
>>> Kris
>>>
>>>  Vladimir
>>>>
>>>> Krystal Mok wrote:
>>>>
>>>>> Thank you for all your input, John and Vladimir!
>>>>> I'm close to getting some prototype that could actually run...but not
>>>>> there just yet.
>>>>> I've got Match.addExact(int, int) compiled down to this graph:
>>>>> http://dl.iteye.com/upload/**picture/pic/114582/cc7535db-**
>>>>> 20d8-3e5d-9abd-b96456c7166c.**png<http://dl.iteye.com/upload/picture/pic/114582/cc7535db-20d8-3e5d-9abd-b96456c7166c.png>
>>>>> with the CheckedAddI node resembling the way DivModI works.
>>>>> But apparently I'm not getting the Matcher part right yet. I'm getting
>>>>> this error message:
>>>>> #  Internal Error (/home/sajia/temp/hotspot-**comp/src/share/vm/opto/*
>>>>> *matcher.cpp:1564), pid=16856, tid=1117243712
>>>>> #  assert(false) failed: bad AD file
>>>>> from this stack trace:
>>>>> V  [libjvm.so+0xb10bea]  VMError::report(outputStream*)**+0xfa4
>>>>> V  [libjvm.so+0xb11f77]  VMError::report_and_die()+**0x649
>>>>> V  [libjvm.so+0x59e706]  report_vm_error(char const*, int, char
>>>>> const*, char const*)+0x9c
>>>>> V  [libjvm.so+0x8d4af0]  Matcher::Label_Root(Node const*, State*,
>>>>> Node*, Node const*)+0x48c
>>>>> V  [libjvm.so+0x8d4d0f]  Matcher::match_tree(Node const*)+0x205
>>>>> V  [libjvm.so+0x8d6397]  Matcher::xform(Node*, int)+0x16d
>>>>> V  [libjvm.so+0x8d9ea2]  Matcher::match()+0xb0e
>>>>> V  [libjvm.so+0x52e20f]  Compile::Code_Gen()+0x91
>>>>> V  [libjvm.so+0x537678]  Compile::Compile(ciEnv*, C2Compiler*,
>>>>> ciMethod*, int, bool, bool)+0x101e
>>>>> V  [libjvm.so+0x47aa8c]  C2Compiler::compile_method(**ciEnv*,
>>>>> ciMethod*, int)+0xea
>>>>> V  [libjvm.so+0x53e552]  CompileBroker::invoke_**compiler_on_method(**
>>>>> CompileTask*)+0x332
>>>>> V  [libjvm.so+0x540f10]  CompileBroker::compiler_**thread_loop()+0x2fa
>>>>> V  [libjvm.so+0xabed88]  compiler_thread_entry(**JavaThread*,
>>>>> Thread*)+0x54
>>>>> V  [libjvm.so+0xac288c]  JavaThread::thread_main_inner(**)+0x102
>>>>> V  [libjvm.so+0xac4f0b]  JavaThread::run()+0xef
>>>>> V  [libjvm.so+0x979557]  java_start(Thread*)+0x16f
>>>>> The current status of the patch is avaiable here:
>>>>> https://gist.github.com/**05f6f33cb01c7f5aedf3<https://gist.github.com/05f6f33cb01c7f5aedf3>
>>>>> I haven't done a full-fledged intrinsic for Math.addExact(int, int)
>>>>> yet; just as a prototype, I'm faking the use of CmpAddI node right now. At
>>>>> the end of Optimize(), I pattern match for the overflow checking logic,
>>>>> replace it with a new fake CmpAddI node, then make the actual CheckedAddI
>>>>> node from the CmpAddI, finally replace the old AddI and Bool nodes.
>>>>> Most of the changes besides the new nodes are for adding the
>>>>> overflow/no-overflow conditions to BoolTest and cmpOp.
>>>>> If there were separate AddI and CmpAddI nodes, they would have had a
>>>>> (Set dst ...) and (Set cr ...) in their match rule. But I can't see how I
>>>>> can express it here.
>>>>> Regards,
>>>>> Kris
>>>>>
>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120621/6deb0585/attachment.html 


More information about the hotspot-compiler-dev mailing list