[ConstantHoisting] Remove dupliate logic in constant hoisting

[aoli]

Summary

As metioned in https://reviews.llvm.org/D34576, checkings in
collectConstantCandidates can be replaced by using
llvm::canReplaceOperandWithVariable.

The only special case is that collectConstantCandidates return false for
all IntrinsicInst but it is safe for us to collect constant candidates from
IntrinsicInst.

[aoli]

Revert mismodified files.

[aoli]

Fix wording.

[efriedma]

lib/Transforms/Scalar/ConstantHoisting.cpp

412 ↗

(On Diff #105295)

Are you sure this is true...? For example, llvm.memcpy has an "len" parameter which can be hoisted, and an "align" parameter which can't.

Or maybe I'm misinterpreting what this comment is talking about?

[aoli]

For alloca instruction, shall we add special case here for constant hoisting? I saw some test cases which test constant hoisting for alloca instruction.

lib/Transforms/Scalar/ConstantHoisting.cpp

412 ↗

(On Diff #105295)

Yes, I checked all implementations of method int getIntImmCost(Intrinsic::ID IID, unsigned Idx, const APInt &Imm, Type *Ty);. There isn't any intrinsic which takes constant parameters only and can not be called freely with constant parameters.

The parameters in llvm.memcpy will never be hoisted because the cost of getting intrinsic immediate is always free for all targets. This is guaranteed in line 310

void ConstantHoistingPass::collectConstantCandidates(
    ConstCandMapType &ConstCandMap, Instruction *Inst, unsigned Idx,
    ConstantInt *ConstInt) {
  unsigned Cost;
  // Ask the target about the cost of materializing the constant for the given
  // instruction and operand index.
  if (auto IntrInst = dyn_cast<IntrinsicInst>(Inst))
    Cost = TTI->getIntImmCost(IntrInst->getIntrinsicID(), Idx,
                              ConstInt->getValue(), ConstInt->getType());
  else
    Cost = TTI->getIntImmCost(Inst->getOpcode(), Idx, ConstInt->getValue(),
                              ConstInt->getType());

  // Ignore cheap integer constants.
  if (Cost > TargetTransformInfo::TCC_Basic) {

[efriedma]

Which testcases involve hoisting from allocas? I don't think we really care what it does as long as it doesn't crash; instcombine canonicalizes allocas with a constant to use the constant "1" anyway.

lib/Transforms/Scalar/ConstantHoisting.cpp

412 ↗

(On Diff #105295)

Oh, that's what you mean by "which can not be freely called with constants". Maybe you could make the comment a bit more explicit (referencing getIntImmCost)?

[aoli]

Update comments.

[aoli]

The test I found is in test/Transforms/ConstantHoisting/ARM/bad-cases.ll

define void @avoid_allocas() {
; CHECK-LABEL: @avoid_allocas
; CHECK: %addr1 = alloca i8, i32 1000
; CHECK: %addr2 = alloca i8, i32 1020

  %addr1 = alloca i8, i32 1000
  %addr2 = alloca i8, i32 1020
  br label %elsewhere

elsewhere:
; CHECK: [[BASE:%.*]] = bitcast i32 1000 to i32
; CHECK: alloca i8, i32 [[BASE]]
; CHECK: [[NEXT:%.*]] = add i32 [[BASE]], 20
; CHECK: alloca i8, i32 [[NEXT]]

  %addr3 = alloca i8, i32 1000
  %addr4 = alloca i8, i32 1020

  ret void
}

[efriedma]

Oh, hmm... maybe canReplaceOperandWithVariable should call isStaticAlloca, or something like that.

[aoli]

if we call isStaticAlloca in canReplaceOperandWithVariable it is still possible to turn a constant-size stack allocation into a variable-size stack allocation in SimplifyCFG. Is that ok?

[aoli]

Remove alloca check in constant hoisting.

[efriedma]

lib/Transforms/Scalar/ConstantHoisting.cpp

399 ↗

(On Diff #105505)

Should canReplaceOperandWithVariable be checking for inline asm?

[aoli]

Add inline asm checks in canReplaceOperandWithVariable.
Add tests for inline asm constant hoisting.

[efriedma]

lib/Transforms/Utils/Local.cpp

2182 ↗

(On Diff #105659)

You can just write "if (isa<InlineAsm>(ImmutableCallSite(I).getCalledValue()))" to handle both Call and Invoke.

[aoli]

Use ImmutableCallSite to get called value.

[efriedma]

LGTM

llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp

 #include "llvm/ADT/Statistic.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/GetElementPtrTypeIterator.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Scalar.h"
+#include "llvm/Transforms/Utils/Local.h"
 #include <tuple>
 
 using namespace llvm;
 using namespace consthoist;
 
 #define DEBUG_TYPE "consthoist"
 
 STATISTIC(NumConstantsHoisted, "Number of constants hoisted");

 /// \brief Scan the instruction for expensive integer constants and record them
 /// in the constant candidate vector.
 void ConstantHoistingPass::collectConstantCandidates(
     ConstCandMapType &ConstCandMap, Instruction *Inst) {
   // Skip all cast instructions. They are visited indirectly later on.
   if (Inst->isCast())
     return;
 
-  // Can't handle inline asm. Skip it.
-  if (auto Call = dyn_cast<CallInst>(Inst))
-    if (isa<InlineAsm>(Call->getCalledValue()))
-      return;
-
-  // Switch cases must remain constant, and if the value being tested is
-  // constant the entire thing should disappear.
-  if (isa<SwitchInst>(Inst))
-    return;
-
-  // Static allocas (constant size in the entry block) are handled by
-  // prologue/epilogue insertion so they're free anyway. We definitely don't
-  // want to make them non-constant.
-  auto AI = dyn_cast<AllocaInst>(Inst);
-  if (AI && AI->isStaticAlloca())
-    return;
-
-  // Constants in GEPs that index into a struct type should not be hoisted.
-  if (isa<GetElementPtrInst>(Inst)) {
-    gep_type_iterator GTI = gep_type_begin(Inst);
-
-    // Collect constant for first operand.
-    collectConstantCandidates(ConstCandMap, Inst, 0);
-    // Scan rest operands.
-    for (unsigned Idx = 1, E = Inst->getNumOperands(); Idx != E; ++Idx, ++GTI) {
-      // Only collect constants that index into a non struct type.
-      if (!GTI.isStruct()) {
-        collectConstantCandidates(ConstCandMap, Inst, Idx);
-      }
-    }
-    return;
-  }
-
   // Scan all operands.
   for (unsigned Idx = 0, E = Inst->getNumOperands(); Idx != E; ++Idx) {
+    // The cost of materializing the constants (defined in
+    // `TargetTransformInfo::getIntImmCost`) for instructions which only take
+    // constant variables is lower than `TargetTransformInfo::TCC_Basic`. So
+    // it's safe for us to collect constant candidates from all IntrinsicInsts.
+    if (canReplaceOperandWithVariable(Inst, Idx) || isa<IntrinsicInst>(Inst)) {
       collectConstantCandidates(ConstCandMap, Inst, Idx);
+    }
   } // end of for all operands
 }
 
 /// \brief Collect all integer constants in the function that cannot be folded
 /// into an instruction itself.
 void ConstantHoistingPass::collectConstantCandidates(Function &Fn) {
   ConstCandMapType ConstCandMap;
   for (BasicBlock &BB : Fn)

llvm/trunk/lib/Transforms/Utils/Local.cpp

bool llvm::canReplaceOperandWithVariable(const Instruction *I, unsigned OpIdx) {
   if (!isa<Constant>(I->getOperand(OpIdx)))
     return true;
 
   switch (I->getOpcode()) {
   default:
     return true;
   case Instruction::Call:
   case Instruction::Invoke:
+    // Can't handle inline asm. Skip it.
+    if (isa<InlineAsm>(ImmutableCallSite(I).getCalledValue()))
+      return false;
     // Many arithmetic intrinsics have no issue taking a
     // variable, however it's hard to distingish these from
     // specials such as @llvm.frameaddress that require a constant.
     if (isa<IntrinsicInst>(I))
       return false;
 
     // Constant bundle operands may need to retain their constant-ness for
     // correctness.

llvm/trunk/test/Transforms/ConstantHoisting/ARM/bad-cases.ll

;CHECK: lshr0 = lshr i96 %a0, 32
   %lshr0 = lshr i96 %a0, 32
   %cast0 = trunc i96 %lshr0 to i32
 ;CHECK: lshr1 = lshr i96 %a1, 32
   %lshr1 = lshr i96 %a1, 32
   %cast1 = trunc i96 %lshr1 to i32
   %ret = add i32 %cast0, %cast1
   ret i32 %ret
 }
+
+@exception_type = external global i8
+
+; Constants in inline ASM should not be hoisted.
+define i32 @inline_asm_invoke() personality i8* null {
+;CHECK-LABEL: @inline_asm_invoke
+;CHECK-NOT: %const = 214672
+;CHECK: %X = invoke i32 asm "bswap $0", "=r,r"(i32 214672)
+  %X = invoke i32 asm "bswap $0", "=r,r"(i32 214672)
+                  to label %L unwind label %lpad
+;CHECK: %Y = invoke i32 asm "bswap $0", "=r,r"(i32 214672)
+  %Y = invoke i32 asm "bswap $0", "=r,r"(i32 214672)
+                  to label %L unwind label %lpad
+L:
+  ret i32 %X
+lpad:
+  %lp = landingpad i32
+      cleanup
+      catch i8* @exception_type
+  ret i32 1
+}
+
+define i32 @inline_asm_call() {
+;CHECK-LABEL: @inline_asm_call
+;CHECK-NOT: %const = 214672
+;CHECK: %X = call i32 asm "bswap $0", "=r,r"(i32 214672)
+  %X = call i32 asm "bswap $0", "=r,r"(i32 214672)
+;CHECK: %Y = call i32 asm "bswap $0", "=r,r"(i32 214672)
+  %Y = call i32 asm "bswap $0", "=r,r"(i32 214672)
+  ret i32 %X
+}

llvm/trunk/test/Transforms/ConstantHoisting/ARM/insertvalue.ll

+; RUN: opt -consthoist -S < %s | FileCheck %s
+target triple = "thumbv6m-none-eabi"
+
+%T = type { i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32,
+i32, i32, i32, i32, i32, i32 }
+
+; The second operand of insertvalue is able to be hoisted.
+define void @test1(%T %P) {
+; CHECK-LABEL:  @test1
+; CHECK:        %const = bitcast i32 256 to i32
+; CHECK:        %1 = insertvalue %T %P, i32 %const, 256
+; CHECK:        %2 = insertvalue %T %P, i32 %const, 256
+  %1 = insertvalue %T %P, i32 256, 256
+  %2 = insertvalue %T %P, i32 256, 256
+  ret void
+}