Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[AMDGPU] Handled G_UBSANTRAP GlobalIsel #134492

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 7 commits into
base: main
Choose a base branch
Loading
from
+177 −15
Open

[AMDGPU] Handled G_UBSANTRAP GlobalIsel #134492

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
515510f
[AMDGPU] Added G_UBSANTRAP GlobalISel
Apr 5, 2025
b92d165
added clang-format output
Apr 5, 2025
626119a
Updated trap.ll and merged debugtrap and ubsantrap legalizers
May 13, 2025
3445c01
Fixed GCN-WARNING mismatch
May 14, 2025
7835b11
Fixed code style
May 14, 2025
8322f92
Updated test
May 14, 2025
3e4b6dd
Style fixes
May 14, 2025
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
Next Next commit
[AMDGPU] Added G_UBSANTRAP GlobalISel
  • Loading branch information
Aman Sharma committed Apr 5, 2025
commit 515510f1ef088d2ed972d4b00e5660e207bb55b0
27 changes: 26 additions & 1 deletion 27 llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2103,7 +2103,7 @@ AMDGPULegalizerInfo::AMDGPULegalizerInfo(const GCNSubtarget &ST_,
getActionDefinitionsBuilder({G_MEMCPY, G_MEMCPY_INLINE, G_MEMMOVE, G_MEMSET})
.lower();

getActionDefinitionsBuilder({G_TRAP, G_DEBUGTRAP}).custom();
getActionDefinitionsBuilder({G_TRAP, G_DEBUGTRAP, G_UBSANTRAP}).custom();
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should add the default lower() action to LegalizerHelper for G_DEBUGTRAP and G_UBSANTRAP, which just replace the opcode with G_TRAP (this is what LegalizeDAG does as the default action)

Copy link
Contributor Author

@amansharma612 amansharma612 May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So should we replace the custom() call to lower() or add lower() as a fallback option?


getActionDefinitionsBuilder({G_VASTART, G_VAARG, G_BRJT, G_JUMP_TABLE,
G_INDEXED_LOAD, G_INDEXED_SEXTLOAD,
Expand Down Expand Up @@ -2222,6 +2222,8 @@ bool AMDGPULegalizerInfo::legalizeCustom(
return legalizeTrap(MI, MRI, B);
case TargetOpcode::G_DEBUGTRAP:
return legalizeDebugTrap(MI, MRI, B);
case TargetOpcode::G_UBSANTRAP:
return legalizeUbsanTrap(MI, MRI, B);
default:
return false;
}
Expand Down Expand Up @@ -7045,6 +7047,29 @@ bool AMDGPULegalizerInfo::legalizeDebugTrap(MachineInstr &MI,
return true;
}


bool AMDGPULegalizerInfo::legalizeUbsanTrap(MachineInstr &MI,
MachineRegisterInfo &MRI,
MachineIRBuilder &B) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is identical to legalizeDebugTrap above, they should just use the same implementation function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trap ID differs in these functions. Is it still required to merge the definitions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can just rename that function above to "legalizeDebugUbsanTrap" and select the TrapID based on the opcode. The warning can also just say "debugtrap/ubsantrap handler not supported".

// Is non-HSA path or trap-handler disabled? Then, report a warning
// accordingly
if (!ST.isTrapHandlerEnabled() ||
ST.getTrapHandlerAbi() != GCNSubtarget::TrapHandlerAbi::AMDHSA) {
DiagnosticInfoUnsupported NoTrap(B.getMF().getFunction(),
"ubsantrap handler not supported",
MI.getDebugLoc(), DS_Warning);
LLVMContext &Ctx = B.getMF().getFunction().getContext();
tgymnich marked this conversation as resolved.
Show resolved Hide resolved
Ctx.diagnose(NoTrap);
} else {
// Insert trap instruction
B.buildInstr(AMDGPU::S_TRAP)
.addImm(static_cast<unsigned>(GCNSubtarget::TrapID::LLVMAMDHSATrap));
}

MI.eraseFromParent();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want it to be removed even trap handler is disabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has to be modified or erased otherwise it will be a legalization loop

return true;
}

bool AMDGPULegalizerInfo::legalizeBVHIntersectRayIntrinsic(
MachineInstr &MI, MachineIRBuilder &B) const {
MachineRegisterInfo &MRI = *B.getMRI();
Expand Down
5 changes: 5 additions & 0 deletions 5 llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,11 @@ class AMDGPULegalizerInfo final : public LegalizerInfo {
bool legalizeDebugTrap(MachineInstr &MI, MachineRegisterInfo &MRI,
MachineIRBuilder &B) const;


bool legalizeUbsanTrap(MachineInstr &MI, MachineRegisterInfo &MRI,
MachineIRBuilder &B) const;


bool legalizeIntrinsic(LegalizerHelper &Helper,
MachineInstr &MI) const override;
};
Expand Down
7 changes: 7 additions & 0 deletions 7 llvm/test/CodeGen/AMDGPU/ubsan_trap.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
; RUN: llc -mtriple=amdgcn-amd-amdhsa -global-isel < %s
; LLVM ERROR: cannot select: G_UBSANTRAP 0 (in function: ubsan_trap)

define void @ubsan_trap() {
call void @llvm.ubsantrap(i8 0)
ret void
}
tgymnich marked this conversation as resolved.
Show resolved Hide resolved
Morty Proxy This is a proxified and sanitized view of the page, visit original site.