[llvm-commits] [PATCH][FastMath, InstCombine] Fadd/Fsub optimizations

[Eli Friedman]

On Fri, Dec 14, 2012 at 10:45 AM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
>   I'd like to resurrect my previous change. See the attachment. It was
> revised per
> your requests. In addition to that, I change the Coeffficient class a bit
> such
> that the cost of default  constructor is merely a 4-byte-store-zero. This
> cost
> renders it possible to "abuse" this data structure.
>
>   As to the determinstic sorting, I have not yet figure out a cost effective
> way
> for that matter. Note that stable_sort() won't help as all Addends sharing
> the same symbolic value will be folded, we don't care their relative
> posittion
> before folding.

We discussed this in person; it sounded like you have a viable solution.

>  Tons of thanks in advance for the code review. (testing-case is not
> included in this
> patch for clarity purpose)

General comment: please try and write as many tests as possible.
Fast-math is generally a new area, so there's very little existing
coverage to depend on.  In particular. please make sure all the
variants of "(x+y) + (y-x)" etc. we discussed are there.

+  I.setHasUnsafeAlgebra(true);
+  if (I.hasUnsafeAlgebra()) {
+    if (Value *V = FAddCombine(Builder).simplify(&I))
+      return ReplaceInstUsesWith(I, V);
+  }

Please remember to remove the I.setHasUnsafeAlgebra for the final patch.

+    bool IsFp;
+
+    // True iff FpValBuf contains a instance of APFloat.
+    bool BufHasFpVal;

How are these different?

+    union {
+      char FpValBuf[sizeof(APFloat)];
+      int dummy; // So this structure has at least 4-byte alignment.
+    };

llvm::AlignedCharArrayUnion.  And I'm assuming you benchmarked and
found this was worth the complexity.

+  // Currently flush-to-0 is ignored. Following stmtement is to suppress
+  // compile-warning.
+  FlushToZero = !FlushToZero;

We generally prefer the pattern "(void)FlushToZero;".

It doesn't look like you addressed my earlier comment about
FAddend::drillDownOneStep.

+    Value *V0 = I->getOperand(0);
+    Value *V1 = I->getOperand(1);
+    InstQuota = ((!isa<Constant>(V0) && V0->hasOneUse()) &&
+                 (!isa<Constant>(V1) && V1->hasOneUse())) ? 2 : 1;

I'm not sure what your logic is here; can you add a comment to explain?

+unsigned FAddend::drillDownOneStep
+  (Value *Val, FAddend &Addend0, FAddend &Addend1) {
+  Instruction *I = 0;
+  if (Val == 0 || !(I = dyn_cast<Instruction>(Val)))
+    return 0;

I'm not sure what you're trying to do with this check.

+  // step 4: Try to optimize Opnd0 + Opnd1_0 [+ Opnd1_1]
+  if (Opnd1_ExpNum) {
+    AddendVect AllOpnds;
+    AllOpnds.push_back(&Opnd0);
+    AllOpnds.push_back(&Opnd1_0);
+    if (Opnd1_ExpNum == 2)
+      AllOpnds.push_back(&Opnd1_1);
+
+    if (Value *R = simplifyFAdd(AllOpnds, 1))
+      return R;
+  }

This seems redundant: why wouldn't it be handled in "step 3"?  Maybe
the way drillDownOneStep works should be changed a little?

-Eli