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

Please see the section under "Sometimes code reviews will take longer than you would hope for" regarding ping time:
https://llvm.org/docs/DeveloperPolicy.html#code-reviews

Also, this patch has grown in functionality since the last rev, but there are still no tests. If you want to demonstrate the effect of propagating the flags, pick just one DAG combine where that happens (ideally, the simplest case) and add tests to show the functional difference.



================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:933
+  /// based optimizations.
+  SDNodeFlags getUnifiedFlags();
+
----------------
I don't think this is going to work as you're hoping for. If possible, please split this and any related changes into a separate follow-up patch.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:666
+  // a given Value.
+  SDNode * getDAGNode(const Value *);
+
----------------
spatel wrote:
> RKSimon wrote:
> > spatel wrote:
> > > Formatting/spacing is non-standard here and below. Run clang-format?
> > is this needed in this patch?
> Same question as earlier (I don't think it was answered). 
> 
> Can we use the existing SelectionDAGBuilder::getValue() to get to the node's flags?
Asking for the 3rd time: is this necessary?


https://reviews.llvm.org/D37686