[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

405

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

405

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

405

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

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

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");
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)

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
+}