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

[Marcello Maggioni]

2014-05-29 23:08 GMT-07:00 Pete Cooper <peter_cooper at apple.com>:

> 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);
>
>
Hmm, I have to admit I copied this from the approach used in the
Operator.h:89 for the "OverflowingBinaryOperator" class.

I thought it was strange, but I assumed it worked because it was there.
Maybe would be a good idea to change it everywhere? (in two separate
patches)


> 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.
>
>
I see :) I'll do it.


> +  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.
>
>
You are right. I originally had tests failing because what came down wasn't
a BinaryOperator (ConstantDataVector for example), but actually casting to
the more specific PossiblyExactOperator or OverflowingBinaryOperator makes
it work without the if! Didn't notice they had the
"hasNoSignedWrap/isExact" methods too (well, it kind of makes sense though
... :P)


> 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.
>
>
Ok, will do!


> 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/20140530/aa7b5c73/attachment.html>