Question on arithmetic overflow detection support in C2

[Vladimir Kozlov]

This looks good.
Add next line only in cmpOp() operand to matching signed.

+ overflow(0x0, "o");
+ no_overflow(0x1, "no");

Do not rename, just use 'n' as argument:

+CheckedAddINode* CheckedAddINode::make(Compile* C, Node* cmpadd) {
+ Node* n = cmpadd;

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()? And did you tried match(Set dst (CheckedAddI dst src))?

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
> 
> 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
>>     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
> 
>