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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

There is a functional difference from this improvement (several FP intrinsics should now correctly propagate flags), but I'm not sure if it's visible without fixing something else in DAGCombiner to recognize the flags.

How does this work if an instruction maps to multiple nodes? For example, the FMA intrinsic can map to 2 nodes?



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:95
+public:
+   SDNodeFlagsAcquirer(const Instruction * I, SelectionDAGBuilder *SDB):
+                       Instr(I), SelDB(SDB) {}
----------------
Formatting/spacing is non-standard here and below. Run clang-format?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:99-100
+  ~SDNodeFlagsAcquirer() {
+      SDNode * Node = SelDB->getDAGNode(Instr);
+      if (Node) {
+        SDNodeFlags Flags = Node->getFlags();
----------------
shorten: if (SDNode *Node = SelDB->getDAGNode(Instr)) {


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7935-7939
   FastMathFlags FMF;
   if (isa<FPMathOperator>(I))
     FMF = I.getFastMathFlags();
   SDNodeFlags SDFlags;
   SDFlags.setNoNaNs(FMF.noNaNs());
----------------
Don't need this anymore?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:666
+  // a given Value.
+  SDNode * getDAGNode(const Value *);
+
----------------
Formatting/spacing is non-standard here and below. Run clang-format?


https://reviews.llvm.org/D37686