[MemCpyOpt] Use memcpy source directly if dest is known to be immutable from attributes

[khei4]

Summary

This patch aims to remove memcpy on immutable arguments from attributes.

We can safely remove src-to-dest memcpy dest is used as only the call parameter if following conditions are satisfied

This is called on memcpy dest pointer arguments attributed as immutable
during call. Try to use memcpy source directly if all of the following
conditions are satisfied.

1. The memcpy dst is neither modified during the call nor captured by the call. (if readonly, noalias, nocapture attributes on call-site.)
2. The memcpy dst is an alloca with known alignment & size.
   1. The memcpy length == the alloca size which ensures that the new pointer is dereferenceable for the required range
   2. The src pointer has alignment >= the alloca alignment or can be enforced so.
3. The memcpy dst and src is not modified between the memcpy and the call. (if MSSA clobber check is safe.)
4. The memcpy src is not modified during the call. (ModRef check shows no Mod.)

(from https://reviews.llvm.org/D150967#4359222 and https://reviews.llvm.org/D150967#4365466 )

precommit tests: https://reviews.llvm.org/D150967
Motivated to resolve https://github.com/rust-lang/rust/issues/107436

[khei4]

update (currently attribute checking not working correctly

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1660

This whole can can be replaced with CB->onlyReadsMemory(i). This will check readonly and readnone and will also correctly handle inheritance of attributes from the function.

1665

CB->paramHasAttr(i, Kind) is enough, it will handle attribute inheritance.

[khei4]

apply feedbacks, remove (most of) debug streams.

[khei4]

@nikic Thank you for the review! They saved me!

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1665

Thank you for saving me! I haven't found the attribute inheritance behavior and am confused about that!

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1647

This check looks unnecessary.

1648–1653

This seems simpler than the loop :)

[khei4]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1647

Thanks! That's remaining for the past diff!

1648–1653

Yeah, agreed, conditions are simpler than before.

[khei4]

attach Differential Revision on comment

[khei4]

process alignment, handle unescaped alloca and arg with noalias, clobber check on Dest of MDep(MemCpy)

[khei4]

need to change test on https://reviews.llvm.org/D150967#4365466 and refine description

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1704–1705

dest check might be redundant because the first clobber check might already show dest is not modified between call and memcpy.

[khei4]

rebase

[khei4]

apply feedback on call with @nikic

1. remove noalias argument or unescaped alloca checks
2. add ModRefCheck for memcpy source

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1627

nit: Start comments with uppercase letter.

1640

nit: auto * when using dyn_cast.

1650

nit: Unnecessary braces.

1661

see -> check that

1663

ImmutArg->stripPointerCasts() is same as AI.

1666

Outdated comment?

1667

byval -> alloca?

1678

Is the explicit MaybeAlign() here needed?

1693

"not modified by the call" maybe?

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1695

MSSA->getMemoryAccess(&CB) is CallAccess.

1697

This second writtenBetween looks redundant to me. This is already ensured by getClobberingMemoryAccess().

1698

Hm, shouldn't this be checking the source? We already know the dest is not modified via readonly noalias.

1708

You can drop the bitcast code, not needed with opaque pointers.

[khei4]

apply feedbacks

[khei4]

@nikic
Thanks for the review! Too much obsolete comments!

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1636–1642

I decided to postpone to the next Revision to collect all possible pointers and check that all ptr are alloca. (it might be a little conservative.)

1693

"not modified by the call" maybe?

Literally right,(writtenBetween guarantees so). TBH, I want to ensure the memcpy source is not modified by another alias during the call.
That part may be done by isModSet I guess.

1695

Good catch!! The same goes for ByVal.

1698

Right! Thanks, I was confused with them.

1708

Thanks! I believe ByVal has it too! Including other comments I'll fix them on ByVal side also!

[khei4]

fix typo

[khei4]

rebase

[khei4]

apply feedbacks.

• fix styles

[khei4]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1678

That was just to adjust arg types to the off-the-shelf util functions! Although it was wrong and removed!

[khei4]

fix styles and comments and postpone detailed case

[nikic]

It looks like the alignment checks are inverted.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1624

<= should be >= here?

1639

"Even if the arg can be based on multiple allocas" or so.

1647

&& !AllocaSize->isScalable() (and a test for this)

1662

"equals to" -> "equals the" or "is equal to the"

1673

Does that work? getZextValue() may assert for wide integers (though probably not really relevant here).

1676

smaller -> larger than or equal

1679

Then you don't need to check !MemDepAlign

1681–1683

1704

stray byval

llvm/test/Transforms/MemCpyOpt/memcpy.ll

444 ↗

(On Diff #528283)

This is not a VLA. You need to do something like alloca i8, i64 %n.

474 ↗

(On Diff #528283)

This case is fine, the problematic case is if the %val alignment is smaller.

[khei4]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1624

Right!

1647

&& !AllocaSize->isScalable()

Sorry, I'm not sure about a scalable vectors pointer could be transformed (maybe ||, not &&?). Do you mean scalable vectors TypeSize are available, but memcpy length cannot be scalable for it, so we couldn't handle it, right?

1673

Works fine! Thanks!

1679

Thank you!

llvm/test/Transforms/MemCpyOpt/memcpy.ll

474 ↗

(On Diff #528283)

Yeah, you seem right. Thank you for the comment!
I also found the alive2 has similar issues :)

https://alive2.llvm.org/ce/z/NZivTQ

[khei4]

apply feedback and inverse alignment vs memcpy length comparison.

also rebase for

• smaller alignment failure case
• address space failure case
• scalable vector failure case

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1639

the all -> all the

1647

Yes, exactly.

1664

Add test with volatile memcpy.

1682

Can drop the extra () here.

1705

byval -> immut

llvm/test/Transforms/MemCpyOpt/memcpy.ll

467 ↗

(On Diff #529169)

I'd just write alloca <vscale x 2 x i32> here. You're using a "homogeneous scalable aggregate" which also tests the right thing, but is a more advanced feature...

473 ↗

(On Diff #529169)

We also need a test where dst is modified between call and memcpy, unless I missed it.

503 ↗

(On Diff #529169)

This comment is outdated.

527 ↗

(On Diff #529169)

We're missing a test where the alignment can be enforced. For that you want to copy from another alloca with lower alignment, and the alignment will be increased by the transform.

[khei4]

rebase
remove redundancy,
fix wrong comments

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1647

Understood. Thanks!

llvm/test/Transforms/MemCpyOpt/memcpy.ll

467 ↗

(On Diff #529169)

Thanks! Yeah as you said in this case, unnecessary complexity!

527 ↗

(On Diff #529169)

Sorry, I'm not sure how this could be. I now found that MemDepAlign is just memcpy's arguments alignment attributes value, so basically enforcing from parameter attributes works, but I couldn't find the way to enforce lower alignment actually!
(I might too naive, but I tried for example,

@g = global ptr null, align 4

define void @immut_param_enforced_alignment(ptr align 16 noalias %val) {
  %val1 = alloca i8, align 16
  store ptr %val, ptr @g
  %p = load ptr, ptr @g
  call void @llvm.memcpy.p0.p0.i64(ptr align 16 %val1, ptr %p, i64 1, i1 false)
  call void @f(ptr nocapture noalias readonly %val1)
  ret void
}

this can't enforce the alignment.)

[nikic]

llvm/test/Transforms/MemCpyOpt/memcpy.ll

527 ↗

(On Diff #529169)

I believe you need something like this:

define void @test() {
  %val = alloca i8, align 1
  store i32 42, ptr %val
  %val1 = alloca i8, align 4
  call void @llvm.memcpy.p0.p0.i64(ptr %val1, ptr %val, i64 1, i1 false)
  call void @f_full_readonly(ptr %val1)
  ret void
}

Here the align 1 on the first alloca will be changed to align 4 by the transform.

[khei4]

rebase for enforced alignment

[khei4]

llvm/test/Transforms/MemCpyOpt/memcpy.ll

527 ↗

(On Diff #529169)

Thank you! Now I see DataLayout specify the alignment for stored types!

[nikic]

llvm/test/Transforms/MemCpyOpt/memcpy.ll

566 ↗

(On Diff #529281)

Hm, so I expected that for this test case the transform would happen and change to align 4 here. Do you know why it doesn't?

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1694

I believe these MSSA->getMemoryAccess(MDep) calls can be replaced by Clobber.

llvm/test/Transforms/MemCpyOpt/memcpy.ll

566 ↗

(On Diff #529281)

Hm, this does work when I test the patch locally, so maybe just outdated test result?

[khei4]

llvm/test/Transforms/MemCpyOpt/memcpy.ll

566 ↗

(On Diff #529281)

Hm, this does work when I test the patch locally, so maybe just outdated test result?

Yeah, Sorry... I have forgotten to build before the test update. I believe this works fine.

[khei4]

update tests

[nikic]

LGTM

[khei4]

rebase

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

 /// Executes one iteration of MemCpyOptPass.
 bool MemCpyOptPass::iterateOnFunction(Function &F) {
   bool MadeChange = false;
 
   // Walk all instruction in the function.
   for (BasicBlock &BB : F) {
     // Skip unreachable blocks. For example processStore assumes that an
     // instruction in a BB can't be dominated by a later instruction in the
     // same BB (which is a scenario that can happen for an unreachable BB that
     // has itself as a predecessor).
     if (!DT->isReachableFromEntry(&BB))
       continue;
 
     for (BasicBlock::iterator BI = BB.begin(), BE = BB.end(); BI != BE;) {
         // Avoid invalidating the iterator.
       Instruction *I = &*BI++;
 
       bool RepeatInstruction = false;
 
       if (auto *SI = dyn_cast<StoreInst>(I))
         MadeChange |= processStore(SI, BI);
       else if (auto *M = dyn_cast<MemSetInst>(I))
         RepeatInstruction = processMemSet(M, BI);
       else if (auto *M = dyn_cast<MemCpyInst>(I))
         RepeatInstruction = processMemCpy(M, BI);
       else if (auto *M = dyn_cast<MemMoveInst>(I))
         RepeatInstruction = processMemMove(M);
       else if (auto *CB = dyn_cast<CallBase>(I)) {
         for (unsigned i = 0, e = CB->arg_size(); i != e; ++i)
           if (CB->isByValArgument(i))
             MadeChange |= processByValArgument(*CB, i);
+        /* else if (CB-> ) attributed with noalias readonly*/
       }
 
       // Reprocess the instruction if desired.
       if (RepeatInstruction) {
         if (BI != BB.begin())
           --BI;
         MadeChange = true;
       }
     }
   }
 
   return MadeChange;
 }
 
 PreservedAnalyses MemCpyOptPass::run(Function &F, FunctionAnalysisManager &AM) {
   auto &TLI = AM.getResult<TargetLibraryAnalysis>(F);
   auto *AA = &AM.getResult<AAManager>(F);
   auto *AC = &AM.getResult<AssumptionAnalysis>(F);
   auto *DT = &AM.getResult<DominatorTreeAnalysis>(F);
   auto *MSSA = &AM.getResult<MemorySSAAnalysis>(F);
 
   bool MadeChange = runImpl(F, &TLI, AA, AC, DT, &MSSA->getMSSA());
   if (!MadeChange)
     return PreservedAnalyses::all();
 
   PreservedAnalyses PA;
   PA.preserveSet<CFGAnalyses>();
   PA.preserve<MemorySSAAnalysis>();
   return PA;
 }
 
 bool MemCpyOptPass::runImpl(Function &F, TargetLibraryInfo *TLI_,
                             AliasAnalysis *AA_, AssumptionCache *AC_,
                             DominatorTree *DT_, MemorySSA *MSSA_) {
   bool MadeChange = false;
   TLI = TLI_;
   AA = AA_;
   AC = AC_;
   DT = DT_;
   MSSA = MSSA_;
   MemorySSAUpdater MSSAU_(MSSA_);
   MSSAU = &MSSAU_;
 
   while (true) {
     if (!iterateOnFunction(F))
       break;
     MadeChange = true;
   }
 
   if (VerifyMemorySSA)
     MSSA_->verifyMemorySSA();
 
   return MadeChange;
 }