This is an archive of the discontinued LLVM Phabricator instance.

[ConstantHoisting] Remove dupliate logic in constant hoisting
ClosedPublic

Authored by aoli on Jun 30 2017, 6:17 PM.

Details

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.

Event Timeline

aoli created this revision.Jun 30 2017, 6:17 PM
aoli updated this revision to Diff 104975.Jun 30 2017, 6:20 PM

Revert mismodified files.

aoli updated this revision to Diff 105295.Jul 5 2017, 10:08 AM

Fix wording.

aoli added a subscriber: llvm-commits.
efriedma added inline comments.Jul 5 2017, 2:13 PM
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 added a comment.Jul 5 2017, 2:35 PM

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 edited edge metadata.Jul 5 2017, 2:40 PM

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 updated this revision to Diff 105343.Jul 5 2017, 3:03 PM

Update comments.

aoli added a comment.Jul 5 2017, 3:05 PM

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
}

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

aoli added a comment.Jul 5 2017, 4:47 PM

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 updated this revision to Diff 105505.Jul 6 2017, 12:08 PM

Remove alloca check in constant hoisting.

efriedma added inline comments.Jul 6 2017, 4:51 PM
lib/Transforms/Scalar/ConstantHoisting.cpp
399

Should canReplaceOperandWithVariable be checking for inline asm?

aoli updated this revision to Diff 105659.Jul 7 2017, 10:12 AM

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

efriedma added inline comments.Jul 7 2017, 11:28 AM
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 updated this revision to Diff 105712.Jul 7 2017, 3:10 PM

Use ImmutableCallSite to get called value.

This revision is now accepted and ready to land.Jul 10 2017, 1:13 PM
This revision was automatically updated to reflect the committed changes.