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