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