AMDGPU: Verify function type matches when matching libcalls #119043

arsenm · 2024-12-06T23:54:08Z

Previously this would recognize a call to a mangled ldexp(float, float)
as a candidate to replace with the intrinsic. We need to verify the second
parameter is in fact an integer.

Fixes: SWDEV-501389

arsenm · 2024-12-06T23:54:25Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

llvmbot · 2024-12-06T23:54:35Z

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

Previously this would recognize a call to a mangled ldexp(float, float)
as a candidate to replace with the intrinsic. We need to verify the second
parameter is in fact an integer.

Fixes: SWDEV-501389

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

4 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp (+1-1)
(modified) llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp (+36-6)
(modified) llvm/lib/Target/AMDGPU/AMDGPULibFunc.h (+21-5)
(modified) llvm/test/CodeGen/AMDGPU/amdgpu-simplify-libcall-ldexp.ll (+41)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp b/llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
index f2c0be76b771b5..cf8b416d23e50d 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULibCalls.cpp
@@ -654,7 +654,7 @@ bool AMDGPULibCalls::fold(CallInst *CI) {
 
   // Further check the number of arguments to see if they match.
   // TODO: Check calling convention matches too
-  if (!FInfo.isCompatibleSignature(CI->getFunctionType()))
+  if (!FInfo.isCompatibleSignature(*Callee->getParent(), CI->getFunctionType()))
     return false;
 
   LLVM_DEBUG(dbgs() << "AMDIC: try folding " << *CI << '\n');
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp b/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp
index 4c596e37476c4e..c23472b147bcef 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp
@@ -969,7 +969,7 @@ static Type* getIntrinsicParamType(
   return T;
 }
 
-FunctionType *AMDGPUMangledLibFunc::getFunctionType(Module &M) const {
+FunctionType *AMDGPUMangledLibFunc::getFunctionType(const Module &M) const {
   LLVMContext& C = M.getContext();
   std::vector<Type*> Args;
   ParamIterator I(Leads, manglingRules[FuncId]);
@@ -997,9 +997,39 @@ std::string AMDGPUMangledLibFunc::getName() const {
   return std::string(OS.str());
 }
 
-bool AMDGPULibFunc::isCompatibleSignature(const FunctionType *FuncTy) const {
-  // TODO: Validate types make sense
-  return !FuncTy->isVarArg() && FuncTy->getNumParams() == getNumArgs();
+bool AMDGPULibFunc::isCompatibleSignature(const Module &M,
+                                          const FunctionType *CallTy) const {
+  const FunctionType *FuncTy = getFunctionType(M);
+
+  // FIXME: UnmangledFuncInfo does not have any type information other than the
+  // number of arguments.
+  if (!FuncTy)
+    return getNumArgs() == CallTy->getNumParams();
+
+  // Normally the types should exactly match.
+  if (FuncTy == CallTy)
+    return true;
+
+  const unsigned NumParams = FuncTy->getNumParams();
+  if (NumParams != CallTy->getNumParams())
+    return false;
+
+  for (unsigned I = 0; I != NumParams; ++I) {
+    Type *FuncArgTy = FuncTy->getParamType(I);
+    Type *CallArgTy = CallTy->getParamType(I);
+    if (FuncArgTy == CallArgTy)
+      continue;
+
+    // Some cases permit implicit splatting a scalar value to a vector argument.
+    auto *FuncVecTy = dyn_cast<VectorType>(FuncArgTy);
+    if (FuncVecTy && FuncVecTy->getElementType() == CallArgTy &&
+        allowsImplicitVectorSplat(I))
+      continue;
+
+    return false;
+  }
+
+  return true;
 }
 
 Function *AMDGPULibFunc::getFunction(Module *M, const AMDGPULibFunc &fInfo) {
@@ -1012,7 +1042,7 @@ Function *AMDGPULibFunc::getFunction(Module *M, const AMDGPULibFunc &fInfo) {
   if (F->hasFnAttribute(Attribute::NoBuiltin))
     return nullptr;
 
-  if (!fInfo.isCompatibleSignature(F->getFunctionType()))
+  if (!fInfo.isCompatibleSignature(*M, F->getFunctionType()))
     return nullptr;
 
   return F;
@@ -1028,7 +1058,7 @@ FunctionCallee AMDGPULibFunc::getOrInsertFunction(Module *M,
     if (F->hasFnAttribute(Attribute::NoBuiltin))
       return nullptr;
     if (!F->isDeclaration() &&
-        fInfo.isCompatibleSignature(F->getFunctionType()))
+        fInfo.isCompatibleSignature(*M, F->getFunctionType()))
       return F;
   }
 
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULibFunc.h b/llvm/lib/Target/AMDGPU/AMDGPULibFunc.h
index 10551bee3fa8d4..580ef51b559d80 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULibFunc.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPULibFunc.h
@@ -352,7 +352,7 @@ class AMDGPULibFuncImpl : public AMDGPULibFuncBase {
   void setName(StringRef N) { Name = std::string(N); }
   void setPrefix(ENamePrefix pfx) { FKind = pfx; }
 
-  virtual FunctionType *getFunctionType(Module &M) const = 0;
+  virtual FunctionType *getFunctionType(const Module &M) const = 0;
 
 protected:
   EFuncId FuncId;
@@ -391,8 +391,22 @@ class AMDGPULibFunc : public AMDGPULibFuncBase {
     return Impl->parseFuncName(MangledName);
   }
 
+  /// Return true if it's legal to splat a scalar value passed in parameter \p
+  /// ArgIdx to a vector argument.
+  bool allowsImplicitVectorSplat(int ArgIdx) const {
+    switch (getId()) {
+    case EI_LDEXP:
+      return ArgIdx == 1;
+    case EI_FMIN:
+    case EI_FMAX:
+      return true;
+    default:
+      return false;
+    }
+  }
+
   // Validate the call type matches the expected libfunc type.
-  bool isCompatibleSignature(const FunctionType *FuncTy) const;
+  bool isCompatibleSignature(const Module &M, const FunctionType *FuncTy) const;
 
   /// \return The mangled function name for mangled library functions
   /// and unmangled function name for unmangled library functions.
@@ -401,7 +415,7 @@ class AMDGPULibFunc : public AMDGPULibFuncBase {
   void setName(StringRef N) { Impl->setName(N); }
   void setPrefix(ENamePrefix PFX) { Impl->setPrefix(PFX); }
 
-  FunctionType *getFunctionType(Module &M) const {
+  FunctionType *getFunctionType(const Module &M) const {
     return Impl->getFunctionType(M);
   }
   static Function *getFunction(llvm::Module *M, const AMDGPULibFunc &fInfo);
@@ -428,7 +442,7 @@ class AMDGPUMangledLibFunc : public AMDGPULibFuncImpl {
 
   std::string getName() const override;
   unsigned getNumArgs() const override;
-  FunctionType *getFunctionType(Module &M) const override;
+  FunctionType *getFunctionType(const Module &M) const override;
   static StringRef getUnmangledName(StringRef MangledName);
 
   bool parseFuncName(StringRef &mangledName) override;
@@ -458,7 +472,9 @@ class AMDGPUUnmangledLibFunc : public AMDGPULibFuncImpl {
   }
   std::string getName() const override { return Name; }
   unsigned getNumArgs() const override;
-  FunctionType *getFunctionType(Module &M) const override { return FuncTy; }
+  FunctionType *getFunctionType(const Module &M) const override {
+    return FuncTy;
+  }
 
   bool parseFuncName(StringRef &Name) override;
 
diff --git a/llvm/test/CodeGen/AMDGPU/amdgpu-simplify-libcall-ldexp.ll b/llvm/test/CodeGen/AMDGPU/amdgpu-simplify-libcall-ldexp.ll
index 24082b8c666111..dc275b33b012da 100644
--- a/llvm/test/CodeGen/AMDGPU/amdgpu-simplify-libcall-ldexp.ll
+++ b/llvm/test/CodeGen/AMDGPU/amdgpu-simplify-libcall-ldexp.ll
@@ -242,6 +242,47 @@ define float @test_ldexp_f32_strictfp(float %x, i32 %y) #4 {
   ret float %ldexp
 }
 
+;---------------------------------------------------------------------
+; Invalid signatures
+;---------------------------------------------------------------------
+
+; Declared with wrong type, second argument is float
+declare float @_Z5ldexpff(float noundef, float noundef)
+
+define float @call_wrong_typed_ldexp_f32_second_arg(float %x, float %wrongtype) {
+; CHECK-LABEL: define float @call_wrong_typed_ldexp_f32_second_arg
+; CHECK-SAME: (float [[X:%.*]], float [[WRONGTYPE:%.*]]) {
+; CHECK-NEXT:    [[CALL:%.*]] = call float @_Z5ldexpff(float [[X]], float [[WRONGTYPE]])
+; CHECK-NEXT:    ret float [[CALL]]
+;
+  %call = call float @_Z5ldexpff(float %x, float %wrongtype)
+  ret float %call
+}
+
+declare <2 x float> @_Z5ldexpDv2_fS_(<2 x float>, <2 x float>)
+
+define <2 x float> @call_wrong_typed_ldexp_v2f32_second_arg(<2 x float> %x, <2 x float> %wrongtype) {
+; CHECK-LABEL: define <2 x float> @call_wrong_typed_ldexp_v2f32_second_arg
+; CHECK-SAME: (<2 x float> [[X:%.*]], <2 x float> [[WRONGTYPE:%.*]]) {
+; CHECK-NEXT:    [[CALL:%.*]] = call <2 x float> @_Z5ldexpDv2_fS_(<2 x float> [[X]], <2 x float> [[WRONGTYPE]])
+; CHECK-NEXT:    ret <2 x float> [[CALL]]
+;
+  %call = call <2 x float> @_Z5ldexpDv2_fS_(<2 x float> %x, <2 x float> %wrongtype)
+  ret <2 x float> %call
+}
+
+declare <2 x float> @_Z5ldexpDv2_ff(<2 x float>, float)
+
+define <2 x float> @call_wrong_typed_ldexp_v2f32_f32(<2 x float> %x, float %wrongtype) {
+; CHECK-LABEL: define <2 x float> @call_wrong_typed_ldexp_v2f32_f32
+; CHECK-SAME: (<2 x float> [[X:%.*]], float [[WRONGTYPE:%.*]]) {
+; CHECK-NEXT:    [[CALL:%.*]] = call <2 x float> @_Z5ldexpDv2_ff(<2 x float> [[X]], float [[WRONGTYPE]])
+; CHECK-NEXT:    ret <2 x float> [[CALL]]
+;
+  %call = call <2 x float> @_Z5ldexpDv2_ff(<2 x float> %x, float %wrongtype)
+  ret <2 x float> %call
+}
+
 attributes #0 = { nobuiltin }
 attributes #1 = { "no-builtins" }
 attributes #2 = { nounwind memory(none) }

changpeng

LGTM

llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp

arsenm · 2024-12-16T05:57:13Z

Merge activity

Dec 16, 12:57 AM EST: A user started a stack merge that includes this pull request via Graphite.
Dec 16, 12:59 AM EST: Graphite rebased this pull request as part of a merge.
Dec 16, 1:01 AM EST: A user merged this pull request with Graphite.

Previously this would recognize a call to a mangled ldexp(float, float) as a candidate to replace with the intrinsic. We need to verify the second parameter is in fact an integer. Fixes: SWDEV-501389

jplehr · 2024-12-16T09:36:47Z

I believe this patch turned the HIP buildbot red https://lab.llvm.org/buildbot/#/builders/123/builds/11310
So far, I was unable to reproduce the error locally.
It reaches an unreachable:

Unhandled param type
UNREACHABLE executed at /buildbot/llvm-project/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp:961!
<...>
#10 0x000055d3fc651f98 llvm::AMDGPUMangledLibFunc::getFunctionType(llvm::Module const&) const (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x130ff98)
#11 0x000055d3fc64f7c9 llvm::AMDGPULibFunc::isCompatibleSignature(llvm::Module const&, llvm::FunctionType const*) const (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x130d7c9)
#12 0x000055d3fc649601 llvm::AMDGPULibCalls::fold(llvm::CallInst*) (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x1307601)
#13 0x000055d3fc64a3df llvm::AMDGPUSimplifyLibCallsPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x13083df)

jplehr · 2024-12-16T10:54:29Z

I believe this patch turned the HIP buildbot red https://lab.llvm.org/buildbot/#/builders/123/builds/11310 So far, I was unable to reproduce the error locally. It reaches an unreachable:

Unhandled param type
UNREACHABLE executed at /buildbot/llvm-project/llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp:961!
<...>
#10 0x000055d3fc651f98 llvm::AMDGPUMangledLibFunc::getFunctionType(llvm::Module const&) const (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x130ff98)
#11 0x000055d3fc64f7c9 llvm::AMDGPULibFunc::isCompatibleSignature(llvm::Module const&, llvm::FunctionType const*) const (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x130d7c9)
#12 0x000055d3fc649601 llvm::AMDGPULibCalls::fold(llvm::CallInst*) (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x1307601)
#13 0x000055d3fc64a3df llvm::AMDGPUSimplifyLibCallsPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/buildbot/hip-vega20-0/clang-hip-vega20/llvm/bin/clang-19+0x13083df)

I checked the logs in more depth after not being able to reproduce the issue locally. May be a machine issue. Looking more into it.

arsenm · 2024-12-16T11:40:48Z

I checked the logs in more depth after not being able to reproduce the issue locally. May be a machine issue. Looking more into it.

Any crash in the compiler cannot be a machine issue

#120068

jplehr · 2024-12-16T11:50:50Z

Any crash in the compiler cannot be a machine issue

I still wonder why I cannot reproduce it though.

arsenm · 2024-12-16T12:01:57Z

Any crash in the compiler cannot be a machine issue

I still wonder why I cannot reproduce it though.

Release vs release with asserts build?

jplehr · 2024-12-16T12:15:40Z

Release vs release with asserts build?

That could indeed be the case.

arsenm added the backend:AMDGPU label Dec 6, 2024 — with Graphite App

arsenm requested review from changpeng, Pierre-vh, ssahasra and vpykhtin December 6, 2024 23:54

arsenm marked this pull request as ready for review December 6, 2024 23:54

changpeng approved these changes Dec 7, 2024

View reviewed changes

ssahasra requested changes Dec 9, 2024

View reviewed changes

llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp Outdated Show resolved Hide resolved

arsenm force-pushed the users/arsenm/amdgpu-simplify-libcall-verify-signature branch 2 times, most recently from aff8f72 to 01db72e Compare December 13, 2024 07:34

arsenm mentioned this pull request Dec 13, 2024

AMDGPU: Fix libcall recognition of image array types #119832

Merged

ssahasra approved these changes Dec 16, 2024

View reviewed changes

arsenm added 3 commits December 16, 2024 05:58

AMDGPU: Verify function type matches when matching libcalls

0d0b9ed

Previously this would recognize a call to a mangled ldexp(float, float) as a candidate to replace with the intrinsic. We need to verify the second parameter is in fact an integer. Fixes: SWDEV-501389

Fix crashing when finding type of ocl_image* types

72c0b1c

Revert array cases

8f52a2f

arsenm force-pushed the users/arsenm/amdgpu-simplify-libcall-verify-signature branch from 01db72e to 8f52a2f Compare December 16, 2024 05:59

arsenm merged commit b446c20 into main Dec 16, 2024
5 of 7 checks passed

arsenm deleted the users/arsenm/amdgpu-simplify-libcall-verify-signature branch December 16, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

AMDGPU: Verify function type matches when matching libcalls #119043

AMDGPU: Verify function type matches when matching libcalls #119043

Uh oh!

arsenm commented Dec 6, 2024

Uh oh!

arsenm commented Dec 6, 2024 •

edited

Loading

Uh oh!

llvmbot commented Dec 6, 2024

Uh oh!

changpeng left a comment

Uh oh!

Uh oh!

arsenm commented Dec 16, 2024 •

edited

Loading

Uh oh!

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

arsenm commented Dec 16, 2024

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

arsenm commented Dec 16, 2024

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

Uh oh!

AMDGPU: Verify function type matches when matching libcalls #119043

AMDGPU: Verify function type matches when matching libcalls #119043

Uh oh!

Conversation

arsenm commented Dec 6, 2024

Uh oh!

arsenm commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 6, 2024

Uh oh!

changpeng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arsenm commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

arsenm commented Dec 16, 2024

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

arsenm commented Dec 16, 2024

Uh oh!

jplehr commented Dec 16, 2024

Uh oh!

Uh oh!

arsenm commented Dec 6, 2024 •

edited

Loading

arsenm commented Dec 16, 2024 •

edited

Loading