Question on arithmetic overflow detection support in C2

[Vladimir Kozlov]

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.

> 
>> 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.
> 
>> 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().

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
>>> 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
>>> 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