[PATCH] D12096: [InstCombineAddSub opportunities]: More opportunities to factorize FAdd/FSub when unsafeAlgebra is present for Inst

[David Majnemer via llvm-commits]

majnemer added a comment.

I am not convinced as to why we need this in InstCombine.  This seems inside the purview of Reassociate.

I ran a quarter of your test cases under reassociate twice and it seems to optimize it just fine.  There seems to be a bug in reassociate that requires us running it twice, I think that is the actual bug.


================
Comment at: test/Transforms/InstCombine/FAddFSubAssociativeFactorize.ll:11-18
@@ +10,10 @@
+; CHECK: ret
+define half @faddsubAssoc1(half %a, half %b) {
+  %tmp1 = fmul fast half %b, 0xH4200 ; 3*b
+  %tmp2 = fmul fast half %a, 0xH4500 ; 5*a
+  %tmp3 = fmul fast half %b, 0xH4000 ; 2*b
+  %tmp4 = fadd fast half %tmp2, %tmp1 ; 5 * a + 3 * b
+  %tmp5 = fadd fast half %tmp4, %tmp3 ; (5 * a + 3 * b) + (2 * b)
+  ret half %tmp5 ; = 5 * ( a + b )
+}
+
----------------
Two runs of the reassociate pass achieve the desired result.

================
Comment at: test/Transforms/InstCombine/FAddFSubAssociativeFactorize.ll:24-31
@@ +23,10 @@
+; CHECK: ret
+define half @faddsubAssoc2(half %a, half %b) {
+  %tmp1 = fmul fast half %b, 0xH4200 ; 3*b
+  %tmp2 = fmul fast half %a, 0xH4500 ; 5*a
+  %tmp3 = fmul fast half %b, 0xH4000 ; 2*b
+  %tmp4 = fadd fast half %tmp2, %tmp1 ; 5 * a + 3 * b
+  %tmp5 = fsub fast half %tmp4, %tmp3 ; (5 * a + 3 * b) - (2 * b)
+  ret half %tmp5 ; = 5 * a + b
+}
+
----------------
Two runs of reassociate get this case:
  %tmp2 = fmul fast half %a, 0xH4500
  %tmp6 = fmul fast half %b, 0xH3C00 ; <- this is just a multiply by 1.
  %tmp5 = fadd fast half %tmp6, %tmp2

================
Comment at: test/Transforms/InstCombine/FAddFSubAssociativeFactorize.ll:37-44
@@ +36,10 @@
+; CHECK: ret
+define half @faddsubAssoc3(half %a, half %b) {
+  %tmp1 = fmul fast half %b, 0xH4200 ; 3*b
+  %tmp2 = fmul fast half %a, 0xH4500 ; 5*a
+  %tmp3 = fmul fast half %b, 0xH4000 ; 2*b
+  %tmp4 = fsub fast half %tmp2, %tmp1 ; 5 * a - 3 * b
+  %tmp5 = fadd fast half %tmp4, %tmp3 ; (5 * a - 3 * b) + (2 * b)
+  ret half %tmp5 ; = 5 * a - b
+}
+
----------------
Likewise, two runs of reassociate get this case.

================
Comment at: test/Transforms/InstCombine/FAddFSubAssociativeFactorize.ll:66-73
@@ +65,10 @@
+; CHECK: fmul fast half %1, 0xH4500
+define half @faddsubAssoc5(half %a, half %b) {
+  %tmp1 = fmul fast half %b, 0xH4200 ; 3*b
+  %tmp2 = fmul fast half %a, 0xH4500 ; 5*a
+  %tmp3 = fmul fast half %b, 0xH4000 ; 2*b
+  %tmp4 = fadd fast half %tmp1, %tmp2 ; 3 * b + 5 * a
+  %tmp5 = fadd fast half %tmp3, %tmp4 ; 2 * b + (3 * b + 5 * a)
+  ret half %tmp5 ; = 5 * (a + b)
+}
+
----------------
Two runs also get this case.


http://reviews.llvm.org/D12096