[PATCH] D91576: [MemCpyOpt] Bail on call slot optimization if it merges alias scopes

[Di Mo via Phabricator via llvm-commits]

modimo created this revision.
modimo added reviewers: hfinkel, hoyFB.
Herald added subscribers: llvm-commits, lxfind, JDevlieghere, hiraditya.
Herald added a project: LLVM.
modimo requested review of this revision.

When MemCpyOpt performs call slot optimization it will merge the metadata between the function and the memcpy. As shown in the added test case, if this results in merged alias.scope metadata the resulting merged memcpy will not be considered non-aliasing to instructions that should alias it such as the lifetime.end of the %src in memcpy %dst,%src. This causes future optimizations to get incorrect AA results leading to bad code.

I've made a pin-point conservative fix here to bails on just call slot optimization if alias data could be introduced. We could also strip away or 'intersect' alias.scope on the final merge to still maintain correctness

I believe there's a more fundamental issue here in how alias.scope gets merged. The change that generates the now 'union' operation (D7490 <https://reviews.llvm.org/D7490>) for merging alias.scope shows that previously the operator was 'intersect'. Looking at how getModRefInfo queries alias.scope it's only ever against noalias:

  ModRefInfo ScopedNoAliasAAResult::getModRefInfo(const CallBase *Call1,
                                                  const CallBase *Call2,
                                                  AAQueryInfo &AAQI) {
    if (!EnableScopedNoAlias)
      return AAResultBase::getModRefInfo(Call1, Call2, AAQI);
  
    if (!mayAliasInScopes(Call1->getMetadata(LLVMContext::MD_alias_scope),
                          Call2->getMetadata(LLVMContext::MD_noalias)))
      return ModRefInfo::NoModRef;
  
    if (!mayAliasInScopes(Call2->getMetadata(LLVMContext::MD_alias_scope),
                          Call1->getMetadata(LLVMContext::MD_noalias)))
      return ModRefInfo::NoModRef;
  
    return AAResultBase::getModRefInfo(Call1, Call2, AAQI);
  }

Union makes sense if alias.scopes are directly compared against alias.scopes to make sure we capture the additional dependencies. However, currently it seems that we only ever use an alias.scope to confirm a noalias. Doing union here would then expand the amount of non-aliasing while fundamentally expanding alias scopes should **decrease** the amount of non-aliasing occurring. I'm not certain on this part though so additional context here would be awesome.

One more point here is what occurs when one of the alias.scope is null—we get null as an output. This means that when one alias.scope is null we perform an intersect but if they're both non-null we now perform a union which seems suspect.

The original bug came from a rust bad codegen which used this bad aliasing to perform additional memcpy optimizations such that the %src got forwarded past its lifetime leading to a dereference of garbage data.


https://reviews.llvm.org/D91576

Files:
  llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
  llvm/test/Transforms/MemCpyOpt/callslot_badaa.ll


Index: llvm/test/Transforms/MemCpyOpt/callslot_badaa.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/MemCpyOpt/callslot_badaa.ll
@@ -0,0 +1,25 @@
+; RUN: opt < %s -S -memcpyopt | FileCheck %s
+
+; Make sure callslot optimization does not allow merging which results in
+; bad alias scopes.
+define i8 @test(i8 %input) {
+  %tmp = alloca i8
+  %dst = alloca i8
+  %src = alloca i8
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %tmp, i8* align 8 %src, i64 1, i1 false)
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %dst, i8* align 8 %tmp, i64 1, i1 false)
+  call void @llvm.lifetime.start.p0i8(i64 8, i8* nonnull %src), !noalias !1
+  store i8 %input, i8* %src
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %tmp, i8* align 8 %src, i64 1, i1 false), !alias.scope !0
+  call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %src), !noalias !1
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %dst, i8* align 8 %tmp, i64 1, i1 false), !alias.scope !1
+  %ret_value = load i8, i8* %dst
+  ret i8 %ret_value
+}
+
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i1)
+
+!0 = !{!0}
+!1 = !{!1}
Index: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
@@ -945,6 +945,13 @@
             C->getArgOperand(ArgI)->getType()->getPointerAddressSpace())
       return false;
 
+  // If alias scopes get expanded, bail
+  auto callScope = C->getMetadata(LLVMContext::MD_alias_scope);
+  auto cpyScope = C->getMetadata(LLVMContext::MD_alias_scope);
+  if (MDNode::getMostGenericAliasScope(callScope, cpyScope) != nullptr) {
+    return false;
+  }
+
   // All the checks have passed, so do the transformation.
   bool changedArgument = false;
   for (unsigned ArgI = 0; ArgI < C->arg_size(); ++ArgI)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D91576.305586.patch
Type: text/x-patch
Size: 2074 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201116/68a0b396/attachment.bin>