Question on arithmetic overflow detection support in C2

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Jun 20 19:07:34 PDT 2012


C2 matchs from bottom to top of graph. So first it tries to match If node and 
failed. In second mail I explained that State for Proj node is incorrect for 
your case because ideal_reg() is incorrect:

s->DFA( m->ideal_reg(), m );

Vladimir

Krystal Mok wrote:
> 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 
> <mailto:rednaxelafx at gmail.com>> wrote:
> 
>     On Thu, Jun 21, 2012 at 8:03 AM, Vladimir Kozlov
>     <vladimir.kozlov at oracle.com <mailto: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
>             <mailto: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
>     <http://x86.ad> ... no wonder I couldn't find it when I searched
>     x86_64.ad <http://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
> 
> 
> 


More information about the hotspot-compiler-dev mailing list