[AMDGPU] Handled G_UBSANTRAP GlobalIsel #134492

amansharma612 · Apr 5, 2025

github-actions · Apr 5, 2025

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · Apr 5, 2025

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-amdgpu

Author: None (amansharma612)

Changes

Fixes #131740

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

3 Files Affected:

(modified) llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp (+25-1)
(modified) llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h (+3)
(added) llvm/test/CodeGen/AMDGPU/ubsan_trap.ll (+7)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp b/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
index 275d0193452a5..dd8a010adb6df 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
@@ -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();
 
   getActionDefinitionsBuilder({G_VASTART, G_VAARG, G_BRJT, G_JUMP_TABLE,
         G_INDEXED_LOAD, G_INDEXED_SEXTLOAD,
@@ -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;
   }
@@ -7045,6 +7047,28 @@ bool AMDGPULegalizerInfo::legalizeDebugTrap(MachineInstr &MI,
   return true;
 }
 
+bool AMDGPULegalizerInfo::legalizeUbsanTrap(MachineInstr &MI,
+                                            MachineRegisterInfo &MRI,
+                                            MachineIRBuilder &B) const {
+  // 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();
+    Ctx.diagnose(NoTrap);
+  } else {
+    // Insert trap instruction
+    B.buildInstr(AMDGPU::S_TRAP)
+        .addImm(static_cast<unsigned>(GCNSubtarget::TrapID::LLVMAMDHSATrap));
+  }
+
+  MI.eraseFromParent();
+  return true;
+}
+
 bool AMDGPULegalizerInfo::legalizeBVHIntersectRayIntrinsic(
     MachineInstr &MI, MachineIRBuilder &B) const {
   MachineRegisterInfo &MRI = *B.getMRI();
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h b/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h
index 1f4e02b0d600a..076a66bb6012f 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h
+++ b/llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.h
@@ -244,6 +244,9 @@ 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;
 };
diff --git a/llvm/test/CodeGen/AMDGPU/ubsan_trap.ll b/llvm/test/CodeGen/AMDGPU/ubsan_trap.ll
new file mode 100644
index 0000000000000..9f961031a1ddb
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/ubsan_trap.ll
@@ -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
+}
\ No newline at end of file

amansharma612 · Apr 5, 2025

This is my first pull request here. Would appreciate any feedback on code and the tests. Thanks.

llvm/test/CodeGen/AMDGPU/ubsan_trap.ll

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

shiltian · Apr 7, 2025

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

+        .addImm(static_cast<unsigned>(GCNSubtarget::TrapID::LLVMAMDHSATrap));
+  }
+
+  MI.eraseFromParent();


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

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

shiltian · Apr 7, 2025

llvm/test/CodeGen/AMDGPU/GlobalISel/ubsan_trap.ll

+; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT:    s_trap 2
+; CHECK-NEXT:    s_setpc_b64 s[30:31]
+  call void @llvm.ubsantrap(i8 0)


add negative test

arsenm · Apr 7, 2025

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

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


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

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

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".

arsenm · Apr 7, 2025

llvm/test/CodeGen/AMDGPU/GlobalISel/ubsan_trap.ll

@@ -0,0 +1,13 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -global-isel < %s | FileCheck %s


This should probably just merge in with trap-abis.ll and/or trap.ll. You're not testing both dag (which looks untested) or the various ABI options

Pierre-vh · Apr 7, 2025

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

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


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".

Pierre-vh · Apr 7, 2025

llvm/test/CodeGen/AMDGPU/trap.ll

@@ -1,3 +1,4 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --function ubsantrap --version 5


This test was not autogenerated, it needs to be manually updated
it looks like the script did not do anything anyway

Should any specific manual checks be added to the ubsantrap function in this case?

arsenm

reverse ping

arsenm · Apr 13, 2025

llvm/test/CodeGen/AMDGPU/trap.ll

+;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
+; GCN: {{.*}}
+; GCN-WARNING: {{.*}}
+; HSA-TRAP: {{.*}}
+; MESA-TRAP: {{.*}}
+; NO-HSA-TRAP: {{.*}}
+; NO-MESA-TRAP: {{.*}}
+; NO-TRAP-BIT: {{.*}}
+; NOMESA-TRAP: {{.*}}
+; TRAP-BIT: {{.*}}


Something went wrong here, and there are no checks on the other function

@arsenm I am unaware of how/what checks to add manually for this function, would appreciate inputs regarding it.

This one is a special case you can't generate. You should be able to copy the checks from a debugtrap function

github-actions · May 13, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

arsenm · May 13, 2025

llvm/test/CodeGen/AMDGPU/trap.ll

+; GCN-WARNING-1: warning: <unknown>:0:0: in function hsa_debugtrap void (ptr addrspace(1)): debugtrap handler not supported
+; GCN-WARNING-2: warning: <unknown>:0:0: in function hsa_debugtrap void (ptr addrspace(1)): debugtrap/ubsantrap handler not supported


Should fix the error messages to be consistent instead of checking two different warnings

arsenm · May 13, 2025

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

  // 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(),
-                                     "debugtrap handler not supported",
+                                     "debugtrap/ubsantrap handler not supported",


Don't deviate from the DAG's message

arsenm · May 13, 2025

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

@@ -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();


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)

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

arsenm · May 13, 2025

llvm/test/CodeGen/AMDGPU/trap.ll

+;define amdgpu_kernel void @ubsantrap(ptr addrspace(1) nocapture readonly %arg0) {
+;  store volatile i32 1, ptr addrspace(1) %arg0
+;  call void @llvm.ubsantrap(i8 0)
+;  store volatile i32 2, ptr addrspace(1) %arg0
+;  ret void
+;}


Uncomment this

amansharma612 · May 17, 2025

@arsenm requesting a review on this

[AMDGPU] Added G_UBSANTRAP GlobalISel

515510f

llvmbot added the backend:AMDGPU label Apr 5, 2025

tgymnich reviewed Apr 5, 2025

View reviewed changes

llvm/test/CodeGen/AMDGPU/ubsan_trap.ll Outdated Show resolved Hide resolved

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

amansharma612 force-pushed the ubsantrap branch from 5d1aa6a to 59d461f Compare April 5, 2025 15:10

llvmbot added the llvm:globalisel label Apr 5, 2025

shiltian reviewed Apr 7, 2025

View reviewed changes

arsenm reviewed Apr 7, 2025

View reviewed changes

added clang-format output

b92d165

amansharma612 force-pushed the ubsantrap branch from e6011b1 to b92d165 Compare April 7, 2025 06:46

Pierre-vh reviewed Apr 7, 2025

View reviewed changes

arsenm reviewed Apr 13, 2025

View reviewed changes

Updated trap.ll and merged debugtrap and ubsantrap legalizers

626119a

arsenm reviewed May 13, 2025

View reviewed changes

Aman Sharma added 4 commits May 14, 2025 12:48

Fixed GCN-WARNING mismatch

3445c01

Fixed code style

7835b11

Updated test

8322f92

Style fixes

3e4b6dd

amansharma612 force-pushed the ubsantrap branch from e5739d7 to 3e4b6dd Compare May 14, 2025 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMDGPU] Handled G_UBSANTRAP GlobalIsel #134492

[AMDGPU] Handled G_UBSANTRAP GlobalIsel #134492

amansharma612 commented Apr 5, 2025

github-actions bot commented Apr 5, 2025

llvmbot commented Apr 5, 2025 •

edited

Loading

amansharma612 commented Apr 5, 2025

shiltian Apr 7, 2025

arsenm Apr 7, 2025

shiltian Apr 7, 2025

arsenm Apr 7, 2025

amansharma612 Apr 7, 2025

Pierre-vh Apr 7, 2025

arsenm Apr 7, 2025

Pierre-vh Apr 7, 2025

Pierre-vh Apr 7, 2025

amansharma612 Apr 7, 2025

arsenm left a comment

arsenm Apr 13, 2025

amansharma612 May 13, 2025

arsenm May 13, 2025

github-actions bot commented May 13, 2025 •

edited

Loading

arsenm May 13, 2025

arsenm May 13, 2025

arsenm May 13, 2025

amansharma612 May 13, 2025 •

edited

Loading

arsenm May 13, 2025

amansharma612 commented May 17, 2025

		@@ -1,3 +1,4 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --function ubsantrap --version 5

		; GCN-WARNING-1: warning: <unknown>:0:0: in function hsa_debugtrap void (ptr addrspace(1)): debugtrap handler not supported
		; GCN-WARNING-2: warning: <unknown>:0:0: in function hsa_debugtrap void (ptr addrspace(1)): debugtrap/ubsantrap handler not supported

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

[AMDGPU] Handled G_UBSANTRAP GlobalIsel #134492

Are you sure you want to change the base?

[AMDGPU] Handled G_UBSANTRAP GlobalIsel #134492

Conversation

amansharma612 commented Apr 5, 2025

github-actions bot commented Apr 5, 2025

llvmbot commented Apr 5, 2025 • edited Loading

amansharma612 commented Apr 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsenm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amansharma612 May 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amansharma612 commented May 17, 2025

llvmbot commented Apr 5, 2025 •

edited

Loading

github-actions bot commented May 13, 2025 •

edited

Loading

amansharma612 May 13, 2025 •

edited

Loading