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

Marcello Maggioni hayarms at gmail.com
Thu Jun 5 12:50:44 PDT 2014


Hi Andrea,

answers are inlined!


2014-06-05 7:11 GMT-07:00 Andrea Di Biagio <andrea.dibiagio at gmail.com>:

> Hi Marcello,
>
> I like this new approach more than the previous one.
> I have some comments though (mostly style related). Please, can you
> have a look at them?
>
> //-----
>
> About patch "x86_opt_wrap_v4.patch"
>
> +; Function Attrs: nounwind readnone ssp uwtable
> +define i64 @fact2(i64 %x) #0 {
>
> Please remove the comment. Also, attribute group #0 is not defined!
> Please remove any reference to it.
> (on a different topic: It would be nice to have a warning for cases
> like this one where a non existing attribute group is referenced in
> the IR..).
>
> //-----
>

Will do!


>
> About patch "flag_nodes_v4.patch"
>
> In include/llvm/CodeGen/SelectionDAG.h:
>
> +  /// isBinOpWithFlags - Returns true if the opcode is a binary operation
> +  /// with flags.
> +  static bool isBinOpWithFlags(unsigned Opcode) {
> +    switch (Opcode) {
> +    case BINOP_NODES_W_FLAGS: return true;
> +    default: return false;
> +    }
> +  }
> +
>
> I think you can move that definition in SelectionDAGNodes.h. You can
> simplify the code in method 'BinaryWithFlagsSDNode::classof' If you
> can make that definition visible. For example, method '
> BinaryWithFlagsSDNode ::classof'  could be rewritten as follows:
>
> //
>   static bool classof(const SDNode *N) {
>     return isBinOpWithFlags(N->getOpcode());
> //
>
> Also, if possible, please remove macro BINOP_NODES_W_FLAGS. In my
> opinion it doesn't make the code more readable.
>

About that ... BINOP_NODES_W_FLAGS is used in a couple of places (in
switches) where mainly I cannot avoid to have the case labels there (I
would like to use isBinOpWithFlags() everywhere, but in switches is not
possible and putting the check outside the switch would be less performant,
so I prefer having the cases there.

Do you think just duplicating the cases in those couple of places would be
ok?


>
+  /// isBinOpWithFlags - Returns true if the opcode is a binary operation
> +  /// with flags.
> You could use \brief here.
>
> In lib/CodeGen/SelectionDAG/DAGCombiner.cpp:
>
> -//===-- DAGCombiner.cpp - Implement a DAG node combiner
> -------------------===//
> +                          //===-- DAGCombiner.cpp - Implement a DAG
> node combiner -------------------===//
>
> Please revert that change.
>
> *** blushes ***

:D Will do.


> In method 'DAGCombiner::combine':
>
> +      if (isa<BinaryWithFlagsSDNode>(N)) {
> +        const BinaryWithFlagsSDNode *BinNode =
> cast<BinaryWithFlagsSDNode>(N);
>
> You can use 'dyn_cast' instead of 'isa' + 'cast'.
>

Ok


>
> In SelectionDAG.cpp:
>
> Method SelectionDAG::GetBinarySDNode
>
> +    return FN;
> +  } else {
> +    BinarySDNode *N = new (NodeAllocator) BinarySDNode(Opcode,
> DL.getIROrder(),
> +                                                       DL.getDebugLoc(),
> VTs,
> +                                                       N1, N2);
> +    return N;
> +  }
>
> Don't use 'else' after a return.
>
> Ok

> In SelectionDAGBuilder.cpp:
> (methods SelectionDAGBuilder::visitShift and
> SelectionDAGBuilder::visitBinary)
>
> +  const OverflowingBinaryOperator *OFBinOp =
> +    dyn_cast<const OverflowingBinaryOperator>(&I);
> +  const PossiblyExactOperator *ExactOp =
> +    dyn_cast<const PossiblyExactOperator>(&I);
> +
> +  const bool IsOverflowBinOperator = OFBinOp != nullptr;
> +  const bool IsExactOperator = ExactOp != nullptr;
> +  const bool nuw = IsOverflowBinOperator ?
> OFBinOp->hasNoUnsignedWrap() : false;
> +  const bool nsw = IsOverflowBinOperator ? OFBinOp->hasNoSignedWrap() :
> false;
> +  const bool exact = IsExactOperator ? ExactOp->isExact() : false;
>
> Instead of using a dyn_cast + check against nullptr + ternary
> conditional operator,
> wouldn't be more readable if you write something like this?
>
> ///////////
> bool nuw = false;
> bool nsw = false;
> bool exact = false;
> if (OverflowingBinaryOperator *OFBinOp =
>     dyn_cast<const OverflowingBinaryOperator>(&I)) {
>   nuw =  OFBinOp->hasNoUnsignedWrap();
>   nsw =  OFBinOp->hasNoSignedWrap() ;
> }
> if (PossiblyExactOperator *ExactOp =
>     dyn_cast<const PossiblyExactOperator>(&I))
>    exact =  ExactOp->isExact();
> //////////
>
> Seems nicer! Thanks.


> In your new test X86/2014-05-30-CombineAddNSW.ll:
>
> +; Function Attrs: nounwind readnone ssp uwtable
> +define i32 @foo(i32 %a, i32 %b) #0 {
>
> Please remove the comment and the reference to attribute group #0.
> Also, please add a brief comment explaining why you expect an 'add'
> instead of a 'xor' in this case.
>
>
Will do.

> Thanks,
> Andrea
>


Cheers,
Marcello
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140605/51f9a994/attachment.html>


More information about the llvm-commits mailing list