Page MenuHomePhabricator

[MemCpyOpt] Don't shorten memset if destination observable through unwinding
ClosedPublic

Authored by nikic on Oct 10 2020, 8:54 AM.

Details

Summary

MemCpyOpt can shorten a memset if it is later partially overwritten by a memcpy. It checks that the destination is not read in between, but we also need to make sure that the destination cannot be observed via unwinding.

As a side-note, at least for the constant size case this transform should really be performed by DSE, and it does have some support for this (isShortenableAtTheBeginning), but apparently doesn't catch the memset+memcpy combination.

Diff Detail

Event Timeline

nikic created this revision.Oct 10 2020, 8:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2020, 8:54 AM
nikic requested review of this revision.Oct 10 2020, 8:54 AM
jdoerfert added inline comments.Oct 10 2020, 9:20 AM
llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1061

If nounwind is set this is not necessary.

nikic added a subscriber: fhahn.Oct 10 2020, 9:25 AM

As a side-note, at least for the constant size case this transform should really be performed by DSE, and it does have some support for this (isShortenableAtTheBeginning), but apparently doesn't catch the memset+memcpy combination.

After looking into this a bit more, the problem is that I tried something like this

define void @test(i8* %src, i8* %dst, i8 %c) {
  call void @llvm.memset.p0i8.i64(i8* %dst, i8 %c, i64 64, i1 false)
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 32, i1 false)
  ret void
}

which doesn't work, while adding a noalias on %dst does work. As a result of D86815, the memcpy now counts as a read-clobber of %dst, as dst and src may be equal. And with that in mind, the MemCpyOpt transform isn't correct either since that change, unless we know that dst and src are NoAlias. cc @fhahn

nikic updated this revision to Diff 297418.Oct 10 2020, 9:31 AM

Add fast path if containing function is nounwind.

jdoerfert accepted this revision.Oct 12 2020, 8:20 AM

Makes sense to me.

This revision is now accepted and ready to land.Oct 12 2020, 8:20 AM