[llvm] 01801d5 - [rs4gc] Fix a latent bug around attribute stripping for intrinsics

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2021-04-19T13:14:07-07:00
New Revision: 01801d5274101273f85b85221392a4e2e2bc574b

URL: https://github.com/llvm/llvm-project/commit/01801d5274101273f85b85221392a4e2e2bc574b
DIFF: https://github.com/llvm/llvm-project/commit/01801d5274101273f85b85221392a4e2e2bc574b.diff

LOG: [rs4gc] Fix a latent bug around attribute stripping for intrinsics

This change fixes a latent bug which was exposed by a change currently in review (https://reviews.llvm.org/D99802#2685032).

The story on this is a bit involved.  Without this change, what ended up happening with the pending review was that we'd strip attributes off intrinsics, and then selectiondag would fail to lower the intrinsic.  Why?  Because the lowering of the intrinsic relies on the presence of the readonly attribute.  We don't have a matcher to select the case where there's a glue node needed.

Now, on the surface, this still seems like a codegen bug.  However, here it gets fun.  I was unable to reproduce this with a standalone test at all, and was pretty much struck until skatkov provided the critical detail.  This reproduces only when RS4GC and codegen are run in the same process and context.  Why?  Because it turns out we can't roundtrip the stripped attribute through serialized IR!

We'll happily print out the missing attribute, but when we parse it back, the auto-upgrade logic has a side effect of blindly overwriting attributes on intrinsics with those specified in Intrinsics.td.  This makes it impossible to exercise SelectionDAG from a standalone test case.

At this point, I decided to treat this an RS4GC bug as a) we don't need to strip in this case, and b) I could write a test which shows the correct behavior to ensure this doesn't break again in the future.

As an aside, I'd originally set out to handle libfuncs too - since in theory they might have the same issues - but backed away quickly when I realized how the semantics of builtin, nobuiltin, and no-builtin-x all interacted.  I'm utterly convinced that no part of the optimizer handles that correctly, and decided not to open that can of worms here.

Added: 
    llvm/test/Transforms/RewriteStatepointsForGC/X86/intrinsic-attributes.ll
    llvm/test/Transforms/RewriteStatepointsForGC/X86/lit.local.cfg

Modified: 
    llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
index 78291b354f88f..369fa8eca65c5 100644
--- a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
+++ b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
@@ -2594,6 +2594,16 @@ static void RemoveNonValidAttrAtIndex(LLVMContext &Ctx, AttrHolder &AH,
 static void stripNonValidAttributesFromPrototype(Function &F) {
   LLVMContext &Ctx = F.getContext();
 
+  // Intrinsics are very delicate.  Lowering sometimes depends the presence
+  // of certain attributes for correctness, but we may have also inferred
+  // additional ones in the abstract machine model which need stripped.  This
+  // assumes that the attributes defined in Intrinsic.td are conservatively
+  // correct for both physical and abstract model.
+  if (Intrinsic::ID id = F.getIntrinsicID()) {
+    F.setAttributes(Intrinsic::getAttributes(Ctx, id));
+    return;
+  }
+
   for (Argument &A : F.args())
     if (isa<PointerType>(A.getType()))
       RemoveNonValidAttrAtIndex(Ctx, F,

diff  --git a/llvm/test/Transforms/RewriteStatepointsForGC/X86/intrinsic-attributes.ll b/llvm/test/Transforms/RewriteStatepointsForGC/X86/intrinsic-attributes.ll
new file mode 100644
index 0000000000000..cc36ecaf3f5ae
--- /dev/null
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/X86/intrinsic-attributes.ll
@@ -0,0 +1,11 @@
+; RUN: opt < %s -S -rewrite-statepoints-for-gc | FileCheck %s
+
+; CHECK: Function Attrs: nounwind readnone
+; CHECK: declare i64 @llvm.x86.sse2.cvttsd2si64(<2 x double>)
+declare i64 @llvm.x86.sse2.cvttsd2si64(<2 x double>)
+
+define i64 @test(<2 x double> %arg) {
+  %ret = call i64 @llvm.x86.sse2.cvttsd2si64(<2 x double> %arg)
+  ret i64 %ret
+}
+

diff  --git a/llvm/test/Transforms/RewriteStatepointsForGC/X86/lit.local.cfg b/llvm/test/Transforms/RewriteStatepointsForGC/X86/lit.local.cfg
new file mode 100644
index 0000000000000..c8625f4d9d248
--- /dev/null
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/X86/lit.local.cfg
@@ -0,0 +1,2 @@
+if not 'X86' in config.root.targets:
+    config.unsupported = True