[PATCH] D12345: [Reassociate]: Add intermediate subtract instructions created while negating to be redone later for more reassociate opportunities

[Chad Rosier via llvm-commits]

mcrosier added a subscriber: mcrosier.
mcrosier added a comment.

Thanks for working on this, Aditya.  I tend to agree with David; I much prefer this solution over the InstCombine equivalent.  I added a few minor nits, but overall this looks good.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:896
@@ -895,2 +895,3 @@
 /// that should be processed next by the reassociation pass.
-static Value *NegateValue(Value *V, Instruction *BI) {
+/// Also adds the intermediate instructions that it creates to be redone at the
+/// end to see if we have more opportunities to reassociate.
----------------
Perhaps,

/// Also add intermediate instructions to the redo list that are modified while pushing the negates through adds.  These will be revisited to see if additional opportunities have been exposed.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:936
@@ +935,3 @@
+    // Add the intermediate negates to the redo list as processing them later
+    // could open up more reassociating opportunities.
+    ToRedo.insert(I);
----------------
open up -> expose

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2106
@@ -2094,1 +2105,3 @@
         Instruction *NI = LowerNegateToMultiply(I);
+        // If the negate got simplified, add the users to be
+        // redone later to see if we can reassociate further (especially
----------------
Perhaps something like:

        // If the negate was simplified, revisit the users to see if we can reassociate further.

================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2130
@@ -2111,1 +2129,3 @@
            !isReassociableOp(I->user_back(), Instruction::FMul))) {
+        // If the negate got simplified, add the users to be
+        // redone later to see if we can reassociate further (especially
----------------
Perhaps something like:

// If the negate was simplified, revisit the users to see if we can reassociate further.

================
Comment at: test/Transforms/Reassociate/reassoc-intermediate-fnegs.ll:2
@@ +1,3 @@
+; RUN: opt < %s -reassociate -S | FileCheck %s
+; CHECK: faddsubAssoc1
+; CHECK: [[TMP1:%tmp.*]] = fmul fast half %a, 0xH4500
----------------
Please use the CHECK-LABEL directive.

================
Comment at: test/Transforms/Reassociate/reassoc-intermediate-fnegs.ll:17
@@ +16,3 @@
+
+; CHECK: faddsubAssoc2
+; CHECK: [[TMP1:%tmp.*]] = fmul fast half %a, 0xH4500
----------------
CHECK-LABEL:


Repository:
  rL LLVM

http://reviews.llvm.org/D12345