[PATCH] Passing down BinaryOperator flags to BinarySDNode + X86 optimization

[Pete Cooper]

Hi Marcello

This is great.  Thanks for working on this.

So the first patch "flag_nodes.patch” contains quite a bit of code cleanup.  Its all good cleanup, but as there’s quite a lot of it can you please separate it out and rebase the other patches on top of the cleanup work.

The * here is a neat trick, but not really valid for a bool.  Would && work the same?

+    SubclassData = 
+      (SubclassData & ~NUW) | (b * NUW);

Inside

+SDValue SelectionDAG::getBinaryNode(unsigned Opcode, SDLoc DL, EVT VT,
+                                    SDValue N1, SDValue N2, bool nuw, bool nsw,
+                                    bool exact)

I think you need to call (and add appropriate code to) AddNodeIDNodeCustom instead of AddNodeIDNode to handle generating the correct hash for subclass data.  See how ISD::LOAD is handled in that method.  A test case to ensure that the same binary operators with different flags aren’t CSEd would be useful to have for this.

+  const BinaryOperator *BinOp = dyn_cast<const BinaryOperator>(&I);
+  // If it is a BinaryOperator use getBinaryNode to create the node
+  if (BinOp != nullptr) {
+    const bool IsOverflowingBin = isa<OverflowingBinaryOperator>(BinOp);
+    const bool IsExactOp = isa<PossiblyExactOperator>(BinOp);
+    const bool nuw = IsOverflowingBin ? BinOp->hasNoUnsignedWrap() : false;
+    const bool nsw = IsOverflowingBin ? BinOp->hasNoSignedWrap() : false;
+    const bool exact = IsExactOp ? BinOp->isExact() : false;
+    SDValue BinNodeValue = DAG.getBinaryNode(
+        OpCode, getCurSDLoc(), Op1.getValueType(), Op1, Op2, nuw, nsw, exact);
+    setValue(&I, BinNodeValue);
+  } else {
+    setValue(&I,
+             DAG.getNode(OpCode, getCurSDLoc(), Op1.getValueType(), Op1, Op2));
+  }

You don’t need the ‘!= nullptr’.  And personally i’d move some of 'OpCode, getCurSDLoc(), …’ up to the same line as DAG.getBinaryNode to make it read easier.  You can also just return after the first setValue and avoid the else branch entirely.

In “x86_opt_wrap.patch”, you have some tabbing which has gone bad, but otherwise the content is good.  Its a shame to see a static_cast not a more LLVM appropriate cast<> or dyn_cast but thats because BinarySDNode doesn’t implement classof and not something that should block your patch right now.  Feel free to fix that later if you want to.

I’d fold the test patch in to the opt patch just so that they are committed together.  The test case could do with being reduced to the minimal possible test to demonstrate this patch (probably just the first BB), but otherwise is good.

Thanks,
Pete

On May 29, 2014, at 7:44 PM, Marcello Maggioni <hayarms at gmail.com> wrote:

> Hello,
> 
> here is a proposed patch to pass down the BinaryOperator's "NoUnsignedWrap/NoSignedWrap and IsExact" flags down to the SDNodes so that backends can use them for additional optimizations.
> 
> The core of the patch is using the SubclassData field of SDNode for BinarySDNodes to store the flags information.
> SelectionDAGBuilder will then set the correct flag information the moment the Binary instruction is visited during construction.
> 
> In doing this I also refactored a little bit the getNode() method accepting two operands, extracting the main constant folding logic into a separate static function.
> 
> The patch implementing this is the file "flag_nodes.patch" attached.
> 
> In addition I implemented an optimization where an unneeded "test instruction" that was emitted from the X86 backend is not emitted anymore. This optimization uses this new feature.
> 
> This optimization is contained in the "x86_opt_wrap.patch" attached.
> 
> I also added a test case that exercises the optimization in the "factorial_test.patch"
> 
> The patch makes this snippet of code:
> 
> LBB0_1:                                 ## %while.body
>                                                ## =>This Inner Loop Header: Depth=1
>          imulq   %rdi, %rax
>          decq    %rdi
>          testq    %rdi, %rdi
>          jg      LBB0_1
> 
> 
> 
> become this:
> 
> 
> 
> LBB0_1:                                 ## %while.body
>                                                ## =>This Inner Loop Header: Depth=1
>          imulq   %rdi, %rax
>          decq    %rdi
>          jg      LBB0_1
> 
> if the NSW flag is set on the SDNode that represents the DEC instruction (presumably an ADD or a SUB node in this case).
> 
> If somebody could review this it would be awesome :)
> 
> Cheers,
> Marcello
> <factorial_test.patch><flag_nodes.patch><x86_opt_wrap.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140529/1c4c8687/attachment.html>