Improve transformation of mul by power-of-2 constants (take 2)

This is a rework of commit f7848c8. Compared to the original change, erroneous
NUW tags are no longer applied to inserted negation instructions - these were
stemming from semantically incorrect attempts to transform `mul nuw` cases for
negative constant operands.

This mul-to-shl optimization was initially introduced in commit d2e586f. From a
functional standpoint, this change addresses the fundamental issue with
`nuw`/`nsw` tags being discarded altogether, when we should in fact respect the
tags for all of the newly generated instructions. We should also skip the
transformation for 1/-1, as apparently this can skew some `mad`-based
optimization opportunities in i64 emulation cases.

Tests are adjusted and expanded accordingly. Additionally, early exits are
employed where appropriate, with more detailed TODOs for future improvements.

Author: Artem Gindinson

IGC/Compiler/CustomSafeOptPass.cpp

@@ -62,8 +62,6 @@ cmp+sel to avoid expensive VxH mov.
 #include "GenISAIntrinsics/GenIntrinsicInst.h"
 #include "common/IGCConstantFolder.h"
 #include "common/LLVMWarningsPush.hpp"
-#include "llvm/Config/llvm-config.h"
-#include <llvmWrapper/ADT/APInt.h>
 #include "llvmWrapper/IR/IntrinsicInst.h"
 #include <llvmWrapper/IR/DIBuilder.h>
 #include <llvmWrapper/IR/DerivedTypes.h>
@@ -73,6 +71,7 @@ cmp+sel to avoid expensive VxH mov.
 #include <llvm/ADT/Statistic.h>
 #include <llvm/ADT/SetVector.h>
 #include <llvm/Analysis/ConstantFolding.h>
+#include <llvm/Analysis/InstructionSimplify.h>
 #include <llvm/IR/Constants.h>
 #include "llvm/IR/DebugInfo.h"
 #include <llvm/IR/Function.h>
@@ -2737,50 +2736,84 @@ void GenSpecificPattern::visitMul(llvm::BinaryOperator& I)
     Value* ValOp = nullptr;
     const APInt* ConstOp = nullptr;
     using namespace llvm::PatternMatch;
-    if (match(&I, m_c_Mul(m_Value(ValOp), m_APInt(ConstOp))))
-    {
-        IRBuilder<> builder(&I);
-        if (ConstOp->isPowerOf2())
-        {
-            I.replaceAllUsesWith(
-                builder.CreateShl(ValOp, (uint64_t)ConstOp->exactLogBase2()));
-            I.eraseFromParent();
-            return;
-        }
-        else if (!IGCLLVM::isNegatedPowerOf2(*ConstOp))
-            return;
+    if (!match(&I, m_c_Mul(m_Value(ValOp), m_APInt(ConstOp))))
+        return;
+
+    // Skip the optimization for mul(x, {1; -1}) - in most cases, such
+    // instances are generated within i64 emulation sequences, and the emitter
+    // is geared toward such patterns.
+    if (ConstOp->isOne() || ConstOp->isAllOnes())
+        return;
 
-        APInt ConstOpAbs = ConstOp->abs();
+    const bool HasNUW = I.hasNoUnsignedWrap();
+    const bool HasNSW = I.hasNoSignedWrap();
+    IGCLLVM::IRBuilder<> builder(&I);
+    // 2^n case
+    if (ConstOp->isPowerOf2())
+    {
         Value* Shl = builder.CreateShl(
-            ValOp, (uint64_t)ConstOpAbs.exactLogBase2());
-        for (User* UI : I.users())
-        {
-            // We're going a little further and making sure that we merge the
-            // shift result's negation with any subsequent adds:
-            //   '%shift = shl i64 %x, n'
-            //   '%res' = sub %var, %shift
-            // preventing the additional negation in between the shift and the
-            // `var` addition. This kind of a peephole optimization may not be
-            // available later down the pipeline.
-            // TODO: Consider moving this logic to add/sub visitors once the
-            // whole GenSpecificPattern iteration logic is guaranteed to allow
-            // deferred instructions.
-            Value* Addend = nullptr;
-            if (match(UI, m_c_Add(m_Specific(&I), m_Value(Addend))))
-            {
-                // Propagate the 'shl' <- 'sub' results instead. No way to
-                // erase the original adds within this pass just yet, as we'd
-                // be invalidating the InstVisitor iteration, but subsequent
-                // DCE instances will handle the orphaned instructions anyway.
-                builder.SetInsertPoint(cast<Instruction>(UI));
-                UI->replaceAllUsesWith(builder.CreateSub(Addend, Shl));
-            }
-        }
-        // Make sure to reset the insertion point for the shl negation after
-        // the possible transformations above
-        builder.SetInsertPoint(&I);
-        I.replaceAllUsesWith(builder.CreateNeg(Shl));
+            ValOp, (uint64_t)ConstOp->exactLogBase2(), "",
+            /*bool NUW=*/HasNUW, /*bool NSW=*/HasNSW);
+        I.replaceAllUsesWith(Shl);
         I.eraseFromParent();
+        return;
+    }
+
+    // -2^n case - legality checks
+    if (!ConstOp->isNegatedPowerOf2())
+        return;
+    // We cannot retain NUW semantics on a negation reliably (LLVM rightly
+    // folds 'sub nuw 0, %y' into 0). Therefore, a transformation like
+    // 'mul nuw %x, -2^n' -> 'shl nuw %x, n'; 'sub 0, %shl'
+    // would be semantically incorrect, and should be skipped.
+    if (HasNUW)
+        return;
+
+    // -2^n case - implementation
+    APInt ConstOpAbs = ConstOp->abs();
+    Value* Shl = builder.CreateShl(
+        ValOp, (uint64_t)ConstOpAbs.exactLogBase2(), "",
+        /*bool NUW=*/HasNUW, /*bool NSW=*/HasNSW);
+    Value* Neg = builder.CreateNegNoNUW(Shl, "", /*bool NSW=*/HasNSW);
+    I.replaceAllUsesWith(Neg);
+    I.eraseFromParent();
+
+    // Go a little further and make sure to merge the shift result's negation
+    // with any subsequent adds:
+    //   '%shift = shl i64 %x, n'
+    //   '%res' = sub %var, %shift
+    // preventing the additional negation in between the shift and `var`
+    // addition.
+    //
+    // TODO: Consider also folding sdiv/srem uses with a constant operand by
+    // negating said operand and using the 'shl' result directly. No reason to
+    // consider 'mul's though - in case of matching overflow flags, we'd expect
+    // these to be constant-folded with the original 'mul' earlier in the
+    // pipeline.
+    //
+    // TODO: Consider re-using an LLVM routine of some sort for this
+    // optimization. That said, on LLVM 14, llvm::SimplifyBinOp() and
+    // higher-level IRBuilder<InstSimplifyFolder>::CreateAdd()/CreateBinOp()
+    // replacements fail to work out of the box.
+    // Alternatively, move this logic to add/sub visitors once the whole
+    // GenSpecificPattern iteration logic allows for deferred instructions.
+    for (User* UI : Neg->users())
+    {
+        Value* Addend = nullptr;
+        if (!match(UI, m_c_Add(m_Specific(Neg), m_Value(Addend))))
+            continue;
+        // Propagate the 'shl' <- 'sub' results instead. No way to erase the
+        // original adds within this pass just yet, as we'd invalidate the
+        // ongoing InstVisitor iteration, however subsequent DCE instances will
+        // handle the orphaned instructions anyway.
+        auto* AddI = cast<Instruction>(UI);
+        if (AddI->hasNoSignedWrap() != HasNSW)
+            continue;
+
+        builder.SetInsertPoint(AddI);
+        Value* Sub = builder.CreateSub(Addend, Shl, "",
+            /*bool NUW=*/AddI->hasNoUnsignedWrap(), /*bool NSW=*/HasNSW);
+        AddI->replaceAllUsesWith(Sub);
     }
 }
 

IGC/Compiler/tests/GenSpecificPattern/binary-typed-pointers.ll

@@ -102,50 +102,5 @@ define void @test_and64(double %src1) {
   ret void
 }
 
-define void @test_simple_mul_pow2(i32 %arg) {
-  %1 = mul i32 %arg, 16
-  call void @use.i32(i32 %1)
-  ret void
-}
-; CHECK-LABEL: @test_simple_mul_pow2(i32 %arg
-; CHECK:       %[[SHL:.+]] = shl i32 %arg, 4
-; CHECK:       call void @use.i32(i32 %[[SHL]])
-; CHECK:       ret void
-
-define void @test_mul_neg_pow2_with_adds(i32 %arg0, i32 %arg1, i32 %arg2) {
-entry:
-; COM: The mul itself + simple use
-  %0 = mul i32 %arg0, -1024
-  call void @use.i32(i32 %0)
-  br label %.add1
-.add1:
-; COM: mul + addend
-  %1 = add i32 %0, %arg1
-  call void @use.i32(i32 %1)
-  br label %.add2
-.add2:
-; COM: addend + mul
-  %2 = add i32 %arg2, %0
-  call void @use.i32(i32 %2)
-  ret void
-}
-; CHECK-LABEL: @test_mul_neg_pow2_with_adds(i32 %arg0, i32 %arg1, i32 %arg2
-; COM: The mul itself + simple use
-; CHECK:     entry:
-; CHECK:       %[[SHL:.+]] = shl i32 %arg0, 10
-; CHECK:       %[[NEG:.+]] = sub i32 0, %[[SHL]]
-; CHECK:       call void @use.i32(i32 %[[NEG]])
-; CHECK:     br label %.add1
-; COM: mul + addend -> addend - shl
-; CHECK:     .add1:
-; CHECK:       %[[SUB1:.+]] = sub i32 %arg1, %[[SHL]]
-; CHECK:       call void @use.i32(i32 %[[SUB1]])
-; CHECK:       br label %.add2
-; COM: addend + mul -> addend - shl
-; CHECK:     .add2:
-; CHECK:       %[[SUB2:.+]] = sub i32 %arg2, %[[SHL]]
-; CHECK:       call void @use.i32(i32 %[[SUB2]])
-; CHECK:       ret void
-
 declare void @use.i64(i64)
 declare void @use.i32(i32)

IGC/Compiler/tests/GenSpecificPattern/binary.ll

@@ -104,50 +104,5 @@ define void @test_and64(double %src1) {
   ret void
 }
 
-define void @test_simple_mul_pow2(i32 %arg) {
-  %1 = mul i32 %arg, 16
-  call void @use.i32(i32 %1)
-  ret void
-}
-; CHECK-LABEL: @test_simple_mul_pow2(i32 %arg
-; CHECK:       %[[SHL:.+]] = shl i32 %arg, 4
-; CHECK:       call void @use.i32(i32 %[[SHL]])
-; CHECK:       ret void
-
-define void @test_mul_neg_pow2_with_adds(i32 %arg0, i32 %arg1, i32 %arg2) {
-entry:
-; COM: The mul itself + simple use
-  %0 = mul i32 %arg0, -1024
-  call void @use.i32(i32 %0)
-  br label %.add1
-.add1:
-; COM: mul + addend
-  %1 = add i32 %0, %arg1
-  call void @use.i32(i32 %1)
-  br label %.add2
-.add2:
-; COM: addend + mul
-  %2 = add i32 %arg2, %0
-  call void @use.i32(i32 %2)
-  ret void
-}
-; CHECK-LABEL: @test_mul_neg_pow2_with_adds(i32 %arg0, i32 %arg1, i32 %arg2
-; COM: The mul itself + simple use
-; CHECK:     entry:
-; CHECK:       %[[SHL:.+]] = shl i32 %arg0, 10
-; CHECK:       %[[NEG:.+]] = sub i32 0, %[[SHL]]
-; CHECK:       call void @use.i32(i32 %[[NEG]])
-; CHECK:     br label %.add1
-; COM: mul + addend -> addend - shl
-; CHECK:     .add1:
-; CHECK:       %[[SUB1:.+]] = sub i32 %arg1, %[[SHL]]
-; CHECK:       call void @use.i32(i32 %[[SUB1]])
-; CHECK:       br label %.add2
-; COM: addend + mul -> addend - shl
-; CHECK:     .add2:
-; CHECK:       %[[SUB2:.+]] = sub i32 %arg2, %[[SHL]]
-; CHECK:       call void @use.i32(i32 %[[SUB2]])
-; CHECK:       ret void
-
 declare void @use.i64(i64)
 declare void @use.i32(i32)