[clang][FMV] Fix crash with cpu_specific attribute.

[labrinea]

When dealing with cpu_specific GlobalDecl, GetOrCreateMultiVersionResolver should immediately return the already created llvm function if it exists.

Fixes #115299.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Alexandros Lamprineas (labrinea)

Changes

Raised here #115299.

The commit a2d3099 introduced replaceDeclarationWith, but it shouldn't be called by the fallthrough code which handles the cpu_specific attribute.

---

Full diff: https://github.com/llvm/llvm-project/pull/115762.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenModule.cpp (-2)
• (modified) clang/test/CodeGen/attr-cpuspecific.c (+6-6)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index ba376f9ecfacde..27b1ccb8137356 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -4597,8 +4597,6 @@ llvm::Constant *CodeGenModule::GetOrCreateMultiVersionResolver(GlobalDecl GD) {
   assert(isa<llvm::GlobalValue>(Resolver) &&
          "Resolver should be created for the first time");
   SetCommonAttributes(FD, cast<llvm::GlobalValue>(Resolver));
-  if (ResolverGV)
-    replaceDeclarationWith(ResolverGV, Resolver);
   return Resolver;
 }
 
diff --git a/clang/test/CodeGen/attr-cpuspecific.c b/clang/test/CodeGen/attr-cpuspecific.c
index 628892d5809b4e..91f6c9e9e06b88 100644
--- a/clang/test/CodeGen/attr-cpuspecific.c
+++ b/clang/test/CodeGen/attr-cpuspecific.c
@@ -114,8 +114,8 @@ void ThreeVersionsSameAttr(void){}
 // CHECK: define {{.*}}void @ThreeVersionsSameAttr.Z() #[[K]]
 
 ATTR(cpu_specific(knl))
-void CpuSpecificNoDispatch(void) {}
-// CHECK: define {{.*}}void @CpuSpecificNoDispatch.Z() #[[K:[0-9]+]]
+void CpuSpecificNoDispatch(void (*f)(void)) {}
+// CHECK: define {{.*}}void @CpuSpecificNoDispatch.Z(ptr noundef %f) #[[K:[0-9]+]]
 
 ATTR(cpu_dispatch(knl))
 void OrderDispatchUsageSpecific(void);
@@ -151,9 +151,9 @@ void usages(void) {
   ThreeVersionsSameAttr();
   // LINUX: @ThreeVersionsSameAttr.ifunc()
   // WINDOWS: @ThreeVersionsSameAttr()
-  CpuSpecificNoDispatch();
-  // LINUX: @CpuSpecificNoDispatch.ifunc()
-  // WINDOWS: @CpuSpecificNoDispatch()
+  CpuSpecificNoDispatch((void (*)(void))CpuSpecificNoDispatch);
+  // LINUX: @CpuSpecificNoDispatch.ifunc(ptr noundef @CpuSpecificNoDispatch.ifunc)
+  // WINDOWS: @CpuSpecificNoDispatch(ptr noundef @CpuSpecificNoDispatch)
   OrderDispatchUsageSpecific();
   // LINUX: @OrderDispatchUsageSpecific.ifunc()
   // WINDOWS: @OrderDispatchUsageSpecific()
@@ -162,7 +162,7 @@ void usages(void) {
   // WINDOWS: @OrderSpecificUsageDispatch()
 }
 
-// LINUX: declare void @CpuSpecificNoDispatch.ifunc()
+// LINUX: declare void @CpuSpecificNoDispatch.ifunc(ptr)
 
 // has an extra config to emit!
 ATTR(cpu_dispatch(ivybridge, knl, atom))

[labrinea]

Not sure who to add as reviewr on this one.

[phoebewang]

Looks reasonable given a2d3099 is for ifunc only.

[erichkeane]

Hmm... I don't know why that would be. Replacing the declaration seems reasonable/necessary in some cases? Though I'm a little frightened by the test changes as a result

Wouldn't mind it if @Fznamznon took a look though, IIRC, she did a bunch of work with cpu_specific/cpu_dispatch.

[Fznamznon]

Wouldn't mind it if @Fznamznon took a look though, IIRC, she did a bunch of work with cpu_specific/cpu_dispatch.

I don't think I did. I'm not familiar with this area to provide any opinion.

[erichkeane]

Wouldn't mind it if @Fznamznon took a look though, IIRC, she did a bunch of work with cpu_specific/cpu_dispatch.

I don't think I did. I'm not familiar with this area to provide any opinion.

Ah, hrmph... maybe it was @elizabethandrews who did a bunch of multiversioning stuff lately?

[labrinea]

I believe the latest revision is a better fix.

[erichkeane]

Can you better explain what the crash cause is? It isn't clear what the change here is doing.

[labrinea]

Can you better explain what the crash cause is? It isn't clear what the change here is doing.

Oh sorry, I thought it became evident with the latest revision. The reason we are crashing is that we call takeName on the same Value. That is because when replaceDeclarationWith is called from the last callsite in the function, the arguments Resolver and ResolverGV are pointing to the same Value. We should be taking the early exit instead, that's why I am asserting that we shouldn't be at the fallthrough when ResolverGV != null. Does it make sense? I'll give a better description on the commit message.

[elizabethandrews]

Wouldn't mind it if @Fznamznon took a look though, IIRC, she did a bunch of work with cpu_specific/cpu_dispatch.

I don't think I did. I'm not familiar with this area to provide any opinion.

Ah, hrmph... maybe it was @elizabethandrews who did a bunch of multiversioning stuff lately?

I briefly worked on target_clones about a year back. I am not very familiar with requirements for cpu_specific. Having said I (naively) think that this PR looks ok since it seems like it just reverts behavior of cpu_specific to whatever it was before a2d3099#diff-e724febedab9c1a2832bf2056d208ff02ddcb2e6f90b5a653afc9b19ac78a5d7 ?

[labrinea]

ping