Question on arithmetic overflow detection support in C2

Krystal Mok rednaxelafx at gmail.com
Thu Jun 28 19:42:30 PDT 2012


Hi Vladimir,

Thank you for reviewing. Comments inline:

On Thu, Jun 28, 2012 at 10:18 PM, Vladimir Kozlov <
vladimir.kozlov at oracle.com> wrote:

> This looks good.
> Add next line only in cmpOp() operand to matching signed.
>
> + overflow(0x0, "o");
> + no_overflow(0x1, "no");
>
> I tried this, but it doesn't work. There would be an error when building
the VM:

assert fails /home/sajia/temp/hotspot-comp/src/share/vm/adlc/output_c.cpp
2908: Do not support octal or decimal encode constants

which is caused by not assigning anything to the overflow/no_overflow's
encoding in other cmpOps.


> Do not rename, just use 'n' as argument:
>
> +CheckedAddINode* CheckedAddINode::make(Compile* C, Node* cmpadd) {
> + Node* n = cmpadd;
>
> Oops, I missed this one on code cleanup. Thanks!


> Did you tried INT_REG_mask() instead of INT_RAX_REG_mask() in
> checkedAddI_sum_proj_mask() to avoid using bound  register in
> checkedAddI_rReg()?


Yes, I did. I tried this, and the whole checkedAddI_rReg node and its
outgoing data-flow was lost after RA/GCM. And then I realized it was
because the sum projection didn't have a DEF for its result, so it the
whole thing got removed.

With the ver3 patch, Math.addExact would be compiled as:

000   B1: #     B3 B2 <- BLOCK HEAD IS JUNK   Freq: 1
000     # stack bang
        pushq   rbp     # Save rbp
        subq    rsp, #16        # Create frame

00c     movl    RAX, RSI        # spill
00e     addl    RAX, RDX        # int with overflow check
010     jo,us  B3  P=0.000001 C=-1.000000
010
012   B2: #     N1 <- B1  Freq: 0.999999
012     addq    rsp, 16 # Destroy frame
        popq   rbp
        testl  rax, [rip + #offset_to_poll_page]        # Safepoint: poll
for GC

01d     ret
01d
01e   B3: #     N1 <- B1  Freq: 1e-06
01e     movl    RSI, #153       # int
023     call,static  wrapper for: uncommon_trap(reason='unloaded'
action='reinterpret' index='153')
        # java.lang.Math::addExact @ bci:14  L[0]=_ L[1]=_ L[2]=_
        # OopMap{off=40}
028     int3    # ShouldNotReachHere
028

Reverting to not using bound register for the sum projection, it'd become:

000   B1: #     B3 B2 <- BLOCK HEAD IS JUNK   Freq: 1
000     # stack bang
        pushq   rbp     # Save rbp
        subq    rsp, #16        # Create frame

00c     jo,us  B3  P=0.000001 C=-1.000000
00c
00e   B2: #     N1 <- B1  Freq: 0.999999
00e     addq    rsp, 16 # Destroy frame
        popq   rbp
        testl  rax, [rip + #offset_to_poll_page]        # Safepoint: poll
for GC

019     ret
019
01a   B3: #     N1 <- B1  Freq: 1e-06
01a     movl    RSI, #153       # int
01f     call,static  wrapper for: uncommon_trap(reason='unloaded'
action='reinterpret' index='153')
        # java.lang.Math::addExact @ bci:14  L[0]=_ L[1]=_ L[2]=_
        # OopMap{off=36}
024     int3    # ShouldNotReachHere
024


> And did you tried match(Set dst (CheckedAddI dst src))?
>
> Yes, I tried it in conjunction with not using bound register for the sum
projection, but it didn't work. I needed a way to specify the effect for
the projection node so that it'd match up with its source.

Thanks,
Kris


> Thanks,
> Vladimir
>
> Krystal Mok wrote:
>
>> Hi all,
>>
>> Just FYI, I've posted an updated version of the patch here:
>> https://gist.github.com/**db03ab15ef8b76246b84#file_**
>> checked_add_prototype.ver3.**patch<https://gist.github.com/db03ab15ef8b76246b84#file_checked_add_prototype.ver3.patch>
>>
>> It pretty much implements what John suggested in a previous email (quoted
>> below).
>>
>> This version still suffers from a couple of problem mentioned before:
>> 1. It's emitting a jmpConU instead of a jmpCon during instruction
>> selection. Is there a way to force it use jmpCon here?
>> 2. I had to use a fixed register for the sum projection of CheckedAddI,
>> otherwise I couldn't find a way to specify this projection should use the
>> same register as "dst".
>>
>> - Kris
>>
>> On Wed, Jun 20, 2012 at 3:12 AM, John Rose <john.r.rose at oracle.com<mailto:
>> john.r.rose at oracle.com**>> wrote:
>>
>>    On Jun 19, 2012, at 7:07 AM, Krystal Mok wrote:
>>
>>     In the DivMod example, Div and Mod nodes with the same inputs
>>>    start out floating separately, and get fused into a single DivMod
>>>    late in Optimize(). The DivMod node type is a MultiNode with 2
>>>    projections, one for the div and the other for the mod. There's
>>>    special treatment of DivMod in Matcher, and custom logic to match
>>>    the projections.
>>>
>>>    If I add a new node type AddIOverflow following the same model, I
>>>    might get it like this:
>>>    https://gist.github.com/**bdd13666e4796b09f36e<https://gist.github.com/bdd13666e4796b09f36e>
>>>    This node type would have 2 projections just like DivMod, one for
>>>    the add, and the other for the overflow condition.
>>>
>>
>>    (rename INT_CC => INT_CC_PAIR for clarity)
>>
>>     Then there are two questions:
>>>
>>>    1. I'd need another new node type, presumably called
>>>    "CheckAddIOverflow" here, that derives from CmpINode and acts as
>>>    if it produces condition codes. AddI(a, b) and
>>>    CheckAddIOverflow(a, b) float separately, just like the DivMod
>>>    example. They get fused up late in Optimize() into the
>>>    AddIOverflow shown above. Does this sound right?
>>>
>>
>>    Yes, that sounds right.  The names don't feel right, yet.  What's
>>    the best term for a version of Add which is not subject to overflow?
>>     That's what the user will be trying to build.  Maybe a word like
>>    "Checked" or "Safe" is best, rather than focusing on overflow.
>>
>>    Maybe:  AddI(a, b) plus CmpAddI(a, b) => CheckedAddI(a, b) plus two
>>    projections yielding the first two values.
>>    You might be right about adding two new BoolNode states.  That way
>>    you can think in terms of:
>>      if ((c = a + b) != 0) ...
>>    turning into
>>      c=AddI(a,b); cc=CmpAddI(a,b); if(Bool::nz(cc)) ...
>>
>>    And overflow turns into the natural special case supported by the HW.
>>
>>     2. With AddIOverflow going into the Matcher, how should I write
>>>    the match rule in the ad file, to
>>>    match: If(Bool(Proj(AddIOverflow(a, b)).overflow, ne), labl) ?
>>>
>>>    Would it look like: match(If cop (AddIOverflow dest src)) ?
>>>
>>
>>    (You have probably already noticed it; the matcher.cpp comment
>>    "Convert (If (Bool (CmpX A B))) into (If (Bool) (CmpX A B))" is
>>    highly relevant to matching If nodes.  See also uses of BinaryNode.)
>>
>>    Following the DivMod pattern, the matcher (in its current state)
>>    will have to match the CheckedAddI as a stand-alone node.  Subtrees
>>    of matches can only have one use; this is a key matcher invariant.
>>     But every DivMod or CheckedAddI will have two uses:  Its two
>>    projections.
>>
>>    So you'll have:
>>
>>       match(CheckedAddI);
>>
>>    and the pre-existing rule:
>>
>>       If(cop cmp)
>>
>>    The cop will be Bool::ov (or Bool::nz in the example above) and the
>>    cmp will be CheckedAddI.cmp.
>>
>>    You might think about extending the matcher machine to allow rules
>>    which specify the disposition of both projections:
>>
>>      match( (If cop (Proj#1 (CheckedAddI a b))); (Set c (Proj#0
>>    (CheckedAddI a b))));
>>
>>    This would be a Big Project.  Long term, I believe it is worth
>>    thinking about ways to model instructions that do more than one
>>    thing at a time.
>>
>>     If the pair of overflow/non-overflow conditions are in BoolTest
>>>    and in cmpOp, perhaps I wouldn't need to match the If node, and
>>>    could just let jmpCon rule handle it as-is.
>>>    That way I just have to match AddIOverflow itself in the ad file,
>>>    like DivMod.
>>>
>>
>>    -- John
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120629/924722d1/attachment-0001.html 


More information about the hotspot-compiler-dev mailing list