Skip to content

[AMDGPU] Fix .Lfunc_end label placement #127549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+40 −7
Open

[AMDGPU] Fix .Lfunc_end label placement #127549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "Utils/SIDefinesUtils.h"
#include "llvm/Analysis/OptimizationRemarkEmitter.h"
#include "llvm/BinaryFormat/ELF.h"
#include "llvm/CodeGen/AsmPrinterHandler.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
#include "llvm/CodeGen/MachineModuleInfo.h"
#include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
Expand Down Expand Up @@ -90,6 +91,22 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAMDGPUAsmPrinter() {
createAMDGPUAsmPrinterPass);
}

namespace {
class AMDGPUAsmPrinterHandler : public AsmPrinterHandler {
Copy link
Contributor

@arsenm arsenm Feb 18, 2025

Choose a reason for hiding this comment

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

I don't understand why we need this fancy handler thing, I don't see other targets using it

Copy link
Collaborator Author

@rampitec rampitec Feb 18, 2025

Choose a reason for hiding this comment

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

Other targets do not print kernel descriptor and do not switch sections inside emitFunctionBodyEnd too.

Copy link
Collaborator Author

@rampitec rampitec Feb 18, 2025

Choose a reason for hiding this comment

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

I.e., the best thing I could do is to create a custom target hook to emit the descriptor. The hook was already there.

Copy link
Contributor

@arsenm arsenm Feb 18, 2025

Choose a reason for hiding this comment

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

There is already emitFunctionDescriptor, except that's artificially depends on AIX. It is also before the function. I guess it doesn't matter if we print these before or after?

Copy link
Collaborator Author

@rampitec rampitec Feb 18, 2025

Choose a reason for hiding this comment

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

Man, do you really expect me to consider AIX here? The whole this thing is enclosed inside the AMDGPU target. I see this AIX comment irrelevant. Just because you see similar words, it does not mean it is relevant.

Copy link
Collaborator Author

@rampitec rampitec Feb 18, 2025

Choose a reason for hiding this comment

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

I am giving you a clean way to print this size. What are you trying to achieve?

protected:
AMDGPUAsmPrinter *Asm;

public:
AMDGPUAsmPrinterHandler(AMDGPUAsmPrinter *A) : Asm(A) {}

void beginFunction(const MachineFunction *MF) override {}

void endFunction(const MachineFunction *MF) override { Asm->endFunction(MF); }

void endModule() override {}
};
} // End anonymous namespace

AMDGPUAsmPrinter::AMDGPUAsmPrinter(TargetMachine &TM,
std::unique_ptr<MCStreamer> Streamer)
: AsmPrinter(TM, std::move(Streamer)) {
Expand Down Expand Up @@ -209,13 +226,12 @@ void AMDGPUAsmPrinter::emitFunctionBodyStart() {
HSAMetadataStream->emitKernel(*MF, CurrentProgramInfo);
}

void AMDGPUAsmPrinter::emitFunctionBodyEnd() {
void AMDGPUAsmPrinter::endFunction(const MachineFunction *MF) {
const SIMachineFunctionInfo &MFI = *MF->getInfo<SIMachineFunctionInfo>();
if (!MFI.isEntryFunction())
return;

if (TM.getTargetTriple().getOS() != Triple::AMDHSA)
return;
assert(TM.getTargetTriple().getOS() == Triple::AMDHSA);

auto &Streamer = getTargetStreamer()->getStreamer();
auto &Context = Streamer.getContext();
Expand Down Expand Up @@ -351,6 +367,8 @@ bool AMDGPUAsmPrinter::doInitialization(Module &M) {
default:
report_fatal_error("Unexpected code object version");
}

addAsmPrinterHandler(std::make_unique<AMDGPUAsmPrinterHandler>(this));
}

return AsmPrinter::doInitialization(M);
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class AMDGPUAsmPrinter final : public AsmPrinter {

void emitFunctionBodyStart() override;

void emitFunctionBodyEnd() override;
void endFunction(const MachineFunction *MF);

void emitImplicitDef(const MachineInstr *MI) const override;

Expand Down
9 changes: 6 additions & 3 deletions llvm/test/CodeGen/AMDGPU/hsa.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,19 @@
; PRE-GFX10: flat_store_dword v{{\[[0-9]+:[0-9]+\]}}, v{{[0-9]+}}
; GFX10: global_store_{{dword|b32}} v{{\[[0-9]+:[0-9]+\]}}, v{{[0-9]+}}, off

; HSA: s_endpgm
; HSA-NEXT: .Lfunc_end0:
; HSA-NEXT: .size simple, .Lfunc_end0-simple

; HSA: .section .rodata,"a",@progbits

; HSA: .amdhsa_user_sgpr_private_segment_buffer 1
; HSA: .amdhsa_user_sgpr_kernarg_segment_ptr 1

; PRE-GFX10-NOT: .amdhsa_wavefront_size32
; GFX10-W32: .amdhsa_wavefront_size32 1
; GFX10-W64: .amdhsa_wavefront_size32 0

; HSA: .Lfunc_end0:
; HSA: .size simple, .Lfunc_end0-simple

define amdgpu_kernel void @simple(ptr addrspace(1) %out) #0 {
entry:
store i32 0, ptr addrspace(1) %out
Expand Down
12 changes: 12 additions & 0 deletions llvm/test/CodeGen/AMDGPU/stack-realign-kernel.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ define amdgpu_kernel void @max_alignment_128() #0 {
; VI-NEXT: buffer_store_dword v0, off, s[0:3], 0 offset:128
; VI-NEXT: s_waitcnt vmcnt(0)
; VI-NEXT: s_endpgm
; VI-NEXT: .Lfunc_end0:
; VI-NEXT: .size max_alignment_128, .Lfunc_end0-max_alignment_128
; VI-NEXT: .section .rodata,"a"
; VI-NEXT: .p2align 6
; VI-NEXT: .amdhsa_kernel max_alignment_128
Expand Down Expand Up @@ -66,6 +68,8 @@ define amdgpu_kernel void @max_alignment_128() #0 {
; GFX9-NEXT: buffer_store_dword v0, off, s[0:3], 0 offset:128
; GFX9-NEXT: s_waitcnt vmcnt(0)
; GFX9-NEXT: s_endpgm
; GFX9-NEXT: .Lfunc_end0:
; GFX9-NEXT: .size max_alignment_128, .Lfunc_end0-max_alignment_128
; GFX9-NEXT: .section .rodata,"a"
; GFX9-NEXT: .p2align 6
; GFX9-NEXT: .amdhsa_kernel max_alignment_128
Expand Down Expand Up @@ -126,6 +130,8 @@ define amdgpu_kernel void @stackrealign_attr() #1 {
; VI-NEXT: buffer_store_dword v0, off, s[0:3], 0 offset:4
; VI-NEXT: s_waitcnt vmcnt(0)
; VI-NEXT: s_endpgm
; VI-NEXT: .Lfunc_end1:
; VI-NEXT: .size stackrealign_attr, .Lfunc_end1-stackrealign_attr
; VI-NEXT: .section .rodata,"a"
; VI-NEXT: .p2align 6
; VI-NEXT: .amdhsa_kernel stackrealign_attr
Expand Down Expand Up @@ -177,6 +183,8 @@ define amdgpu_kernel void @stackrealign_attr() #1 {
; GFX9-NEXT: buffer_store_dword v0, off, s[0:3], 0 offset:4
; GFX9-NEXT: s_waitcnt vmcnt(0)
; GFX9-NEXT: s_endpgm
; GFX9-NEXT: .Lfunc_end1:
; GFX9-NEXT: .size stackrealign_attr, .Lfunc_end1-stackrealign_attr
; GFX9-NEXT: .section .rodata,"a"
; GFX9-NEXT: .p2align 6
; GFX9-NEXT: .amdhsa_kernel stackrealign_attr
Expand Down Expand Up @@ -237,6 +245,8 @@ define amdgpu_kernel void @alignstack_attr() #2 {
; VI-NEXT: buffer_store_dword v0, off, s[0:3], 0 offset:4
; VI-NEXT: s_waitcnt vmcnt(0)
; VI-NEXT: s_endpgm
; VI-NEXT: .Lfunc_end2:
; VI-NEXT: .size alignstack_attr, .Lfunc_end2-alignstack_attr
; VI-NEXT: .section .rodata,"a"
; VI-NEXT: .p2align 6
; VI-NEXT: .amdhsa_kernel alignstack_attr
Expand Down Expand Up @@ -288,6 +298,8 @@ define amdgpu_kernel void @alignstack_attr() #2 {
; GFX9-NEXT: buffer_store_dword v0, off, s[0:3], 0 offset:4
; GFX9-NEXT: s_waitcnt vmcnt(0)
; GFX9-NEXT: s_endpgm
; GFX9-NEXT: .Lfunc_end2:
; GFX9-NEXT: .size alignstack_attr, .Lfunc_end2-alignstack_attr
; GFX9-NEXT: .section .rodata,"a"
; GFX9-NEXT: .p2align 6
; GFX9-NEXT: .amdhsa_kernel alignstack_attr
Expand Down