Skip to content

Conversation

@jplehr
Copy link
Contributor

@jplehr jplehr commented May 22, 2025

This fixes the errors on our HIP-Kokkos buildbot after 68472a3.

I do not know if this is the actually right fix, but afaict it restores the behavior for AMDGPU before the mentioned patch.

@llvmbot
Copy link
Member

llvmbot commented May 22, 2025

@llvm/pr-subscribers-backend-amdgpu

Author: Jan Patrick Lehr (jplehr)

Changes

This fixes the errors on our HIP-Kokkos buildbot after 68472a3.

I do not know if this is the actually right fix, but afaict it restores the behavior for AMDGPU before the mentioned patch.


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

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp (+10)
diff --git a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp
index 19e394e5bc38c..0c98c32c21c2c 100644
--- a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp
+++ b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp
@@ -18,6 +18,7 @@
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/MC/MCValue.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/TargetParser/TargetParser.h"
@@ -51,6 +52,8 @@ class AMDGPUAsmBackend : public MCAsmBackend {
 
   std::optional<MCFixupKind> getFixupKind(StringRef Name) const override;
   MCFixupKindInfo getFixupKindInfo(MCFixupKind Kind) const override;
+  bool shouldForceRelocation(const MCAssembler &, const MCFixup &,
+                             const MCValue &, const MCSubtargetInfo *) override;
 };
 
 } //End anonymous namespace
@@ -189,6 +192,13 @@ MCFixupKindInfo AMDGPUAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
   return Infos[Kind - FirstTargetFixupKind];
 }
 
+bool AMDGPUAsmBackend::shouldForceRelocation(const MCAssembler &,
+                                             const MCFixup &,
+                                             const MCValue &Target,
+                                             const MCSubtargetInfo *) {
+  return Target.getSpecifier();
+}
+
 unsigned AMDGPUAsmBackend::getMinimumNopSize() const {
   return 4;
 }

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This follows the other targets that were changed, and it presumably fixes the regressions, so I'm going to assume it's correct for now.

@jplehr jplehr merged commit 9d3ce55 into llvm:main May 22, 2025
9 of 12 checks passed
@jplehr jplehr deleted the fix/hip-relocation-after-68472a3 branch May 22, 2025 21:03
Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of errors? This should probably include a test?

@jplehr
Copy link
Contributor Author

jplehr commented May 22, 2025

We had runtime errors on the HIP bots. If this fixes these issues I'm happy to dig more into it and add some smaller test.
(It did fix the tests locally for me)

@MaskRay
Copy link
Member

MaskRay commented May 23, 2025

You may identify which relocatable file is different (llvm-objdump -dr) with this shouldForceRelocation change and then add a test to llvm/test/MC/AMDGPU. llvm/test/MC/Sparc/Relocations/relocation.s, which I recently cleaned up, has a good example

@MaskRay
Copy link
Member

MaskRay commented May 23, 2025

It is likely a PC-relative fixup. Could it be AMDGPU::fixup_si_sopp_br ?

AMDGPU also has GOTPCREL relocations similar to x86: s_mov_b32 s3, global_var1@gotpcrel32@lo. You may want a test that local@gotpcrel32@lo (where local is a local symbol defined in the current section) also forces a relocation. I improved the X86 GOT tests in a3d3931

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants