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

[Andrea Di Biagio]

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

//-----

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.

+  /// 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.

In method 'DAGCombiner::combine':

+      if (isa<BinaryWithFlagsSDNode>(N)) {
+        const BinaryWithFlagsSDNode *BinNode = cast<BinaryWithFlagsSDNode>(N);

You can use 'dyn_cast' instead of 'isa' + 'cast'.

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.

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();
//////////

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.

Thanks,
Andrea