[PATCH] D131672: [instcombine] Optimise for zero initialisation of product given finite math in Clang

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:498
+  Value *Start = nullptr, *Step = nullptr;
+  if (matchSimpleRecurrence(&I, PN, Start, Step) && I.hasNoInfs() &&
+      I.hasNoNaNs() && I.hasNoSignedZeros()) {
----------------
Checking for no-inf is correct, but not strictly necessary since 0.0 * Inf = NaN and we check 'nnan' (see InstSimplify for the existing constraints for the fold of X*0.0 to 0.0). 


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:500
+      I.hasNoNaNs() && I.hasNoSignedZeros()) {
+    if (phiAllConstantOperandsAreZero(*PN)) {
+      auto *ZeroVal = Constant::getNullValue(I.getType());
----------------
We don't need to walk through the phi operands again - the zero we want had to be captured as "Start"?

  if (matchSimpleRecurrence(&I, Phi, Start, Step) && match(Start, m_Zero()) &&
      I.hasNoInfs() && I.hasNoNaNs() && I.hasNoSignedZeros())
    return replaceInstUsesWith(I, Start);



================
Comment at: llvm/test/Transforms/InstCombine/remove-loop-phi-fastmul.ll:23
   %0 = load double, ptr %arrayidx, align 8
   %mul = fmul fast double %f_prod.01, %0
   %inc = add i64 %i.02, 1
----------------
This test should have the minimal FMF needed to trigger the transform.


================
Comment at: llvm/test/Transforms/InstCombine/remove-loop-phi-fastmul.ll:135
 
 define double @test_phi_non_constant_operand(ptr %arr_d, double %entry_const) {
 ; CHECK-LABEL: @test_phi_non_constant_operand(
----------------
I don't think this test is adding value. It just shows the expected X*0.0 simplification to 0.0?

All of the tests should be reduced to the minimum IR needed to show the transform. Looking at existing tests in the file "recurrence.ll" might be useful.

If you want to verify that a larger example reduces with the usual opt pipeline, it would be better to add a test under "test/Transforms/PhaseOrdering".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131672