Question on arithmetic overflow detection support in C2 (original) (raw)

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"); + nooverflow(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 INTREGmask() instead of INTRAXREGmask() in checkedAddIsumprojmask() to avoid using bound register in checkedAddIrReg()?

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 checkedaddprototype.ver3.**patch<https://gist.github.com/db03ab15ef8b76246b84#filecheckedaddprototype.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 INTCC => INTCCPAIR 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