(original) (raw)

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@oracle.com <mailto:john.r.rose@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