[PATCH] D61199: [DAG] Refactor DAGCombiner::ReassociateOps

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I don't mind the test churn, but is there a motivating test that shows that we missed an optimization? If not (and this patch is virtually NFC), then let's include that in the description/commit message.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:461
     SDValue XformToShuffleWithZero(SDNode *N);
+    SDValue ReassociateOpsCommutative(unsigned Opc, const SDLoc &DL, SDValue N0,
+                                      SDValue N1);
----------------
ReassociateOpsCommutative -> reassociateOpsCommutative (lowerCamelCase)

Please fix the existing "ReassociateOps" as part of this patch or as a preliminary cleanup too. If we keep using the wrong formatting based on existing code, we'll never converge on the right formatting. :)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1005
 
-SDValue DAGCombiner::ReassociateOps(unsigned Opc, const SDLoc &DL, SDValue N0,
-                                    SDValue N1, SDNodeFlags Flags) {
+// Helper for DAGCombiner::ReassociateOps. Trying to reassociate an expression
+// such as (Opc N0, N1), if \p N0 is the same kind of operation as \p Opc.
----------------
Trying -> Try


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1039-1041
+SDValue DAGCombiner::ReassociateOps(unsigned Opc, const SDLoc &DL, SDValue N0,
+                                    SDValue N1, SDNodeFlags Flags) {
+  // Don't reassociate reductions.
----------------
I'm not sure if we have a utility function to enforce this, but it would be nice to assert that Opc is actually a commutative opcode.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61199/new/

https://reviews.llvm.org/D61199