[PATCH] D37686: [DAG] Consolidating Instruction->SDNode Flags propagation in one class for better code management.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:396
 
   // These are mutators for each flag.
+  void setNoUnsignedWrap(bool b, bool Commit = true) {
----------------
We need a comment here explaining what the Commit parameter does (and how/when it is used).


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:95
+/// applied during Build DAG phase is eventually merged with the flags
+/// over Instruction. Since a DAG node could be shared b/w multiple Instructions
+/// thus flags held by node are intersection of flags contributed by
----------------
shared b/w -> shared by


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:123
+      }
+
+      if (auto *ExactOp = dyn_cast<const PossiblyExactOperator>(Instr))
----------------
I'm a bit worried about propagating the integer flags automatically this way. Maybe this is fine in practice, but if we're adding some kind of implicit contract here, we should clearly document it. An operation that is exact, or does not overflow in some way, could be implemented in terms of operations that do (and, then, having the flags on those intermediate nodes wouldn't be correct).


https://reviews.llvm.org/D37686