[PATCH] propagate IR-level fast-math-flags to DAG nodes

Andrea Di Biagio Andrea_DiBiagio at sn.scee.net
Thu Apr 9 04:52:21 PDT 2015


Hi Sanjay,

I only had a couple of minor comments/questions. In general I think this is a nice initial patch.
If I remember correctly, the initial support for nsw/nuw/exact flags in SelectionDAG was added by Marcello Maggioni. In case, I suggest you to add him in cc to the code review (I don't know if he is still working on this..).


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1230-1231
@@ -1230,5 +1229,4 @@
 
-  BinarySDNode *GetBinarySDNode(unsigned Opcode, SDLoc DL, SDVTList VTs,
-                                SDValue N1, SDValue N2, bool nuw, bool nsw,
-                                bool exact);
+  SDNode *GetSDNodeWithFlags(unsigned Opcode, SDLoc DL, SDVTList VTs,
+                             ArrayRef<SDValue> Ops, const SDNodeFlags *Flags);
 
----------------
My understanding is that flags can only be present on binary operations. This is also the reason why originally nsw/nuw/exact were only added to BinarySDNode.
Is there a reason why BinarySDNode should extend from SDNode? At the moment, 'Ops' is always expected to have two SDValues.

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:971-977
@@ +970,9 @@
+
+  void setHasNoUnsignedWrap(bool b) {
+    Flags = (Flags & ~NoUnsignedWrap) | (b ? NoUnsignedWrap : 0);
+  }
+  
+  void setHasNoSignedWrap(bool b) {
+    Flags = (Flags & ~NoSignedWrap) | (b ? NoSignedWrap : 0);
+  }
+  
----------------
This is ok. However, what about having something like this?

```
setNoUnsignedWrap() {
  Flags |= NoUnsignedWrap;
}

clearNoUnsignedWrap() {
  Flags ^= NoUnsignedWrap;
}
```

Also it is a shame that most of this code is repeated in 'class SDNodeWithFlags'. I wonder if there is a better design that allows to delegate the 'bit manipulation part' as much as possible to 'SDNodeFlags'...

http://reviews.llvm.org/D8900

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list