AMDGPU : Add trap handler support.

[wdng]

Summary

Add trap handler support.

[wdng]

I am sorry for checking in this code into LLVM repo by mistake. Please let me know if you have any comments for this patch so that I will modify & checkin accordingly. Thanks!

[wdng]

lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

193–194 ↗

(On Diff #85535)

Matts: "This also needs to handle debug.trap(), and the case when unreachable’s are turned into traps. There should also be tests specifically for the annotator."

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

Matt: "Should check for the enabled feature bits in the kernel_code_t. This also doesn’t have anything setting enable_trap_handler"

[wdng]

lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

193–194 ↗

(On Diff #85535)

How should we handle debug.trap()? For the case of unreachable's, should we invoke debug.trap()?

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

I think "@llvm.trap()" will enable_trap_handler, right?

[arsenm]

lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

193–194 ↗

(On Diff #85535)

There is a separate llvm.debugtrap intrinsic. I looked around SelectionDAG for other places that introduce ISD::TRAP. The expansion for ISD::DEBUGTRAP creates a TRAP. unreachable instructions also emit a trap if the DAG.getTarget().Options.TrapUnreachable is enabled.

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

Yes, that is the problem. You need to be setting the trap handler bit

[wdng]

lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

193–194 ↗

(On Diff #85535)

Yes, ISD::DEBUGTRAP has been replaced with ISD::TRAP at the legalization phase. So, do we still need to map llvm.debugtrap intrinsic with amdgpu-queue-ptr?

[tony-tye]

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

I am not sure if we should have the compiler be responsible for setting the enable_trap_handler bit. In general I don't think the compiler can figure this out on a per kernel basis. Once we support function calls and indirect calls how could be know if the closure of all functions include some that need the trap handler?

Also, even if we did set the bit, for compute, the hardware CP microcode cannot do anything with it other than refuse to execute the kernel. Today I believe the CP micro code ignores this bit and always enables the trap handler as it is needed for CWSR (context switching).

So it seems the presence of a trap handler is more a function of the environment that will execute the kernel than the kernel itself.

So perhaps we should simply not define the trap handler bit. The code object is already marked as requiring the HSA environment and perhaps this implies the presence of a trap handler.

I suspect that graphics kernels have their own "environment" they execute in and may not support trap handlers. If they do not then executing an S_TRAP will simply be a NOP which will not halt the shader.

[arsenm]

lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

193–194 ↗

(On Diff #85535)

Yes. You should add a test which uses this. As it is this will fail because the queue ptr won't be enabled

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

I don't think that implies the presence of the trap handler. This is a field in the kernel_code_t, so we should set it. We can figure out a conservative setting in the future whenever indirect calls are needed.

I thought enabling this also required reserving 16 SGPRs?

[wdng]

lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

193–194 ↗

(On Diff #85535)

To enable or disable queue ptr, it depends whether we have used "-mtriple=amdgcn--amdhsa" or not. Since ISD::DEBUGTRAP has been replaced with ISD::TRAP, I don't think the test will fail if we compile llvm.debugtrap with "-mtriple=amdgcn--amdhsa", correct?

[arsenm]

lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

193–194 ↗

(On Diff #85535)

That occurs in the DAG. Here you are trying to identify all the situations where ISD::TRAP will be emitted from the IR. Eventually I want to replace how the ABI is lowered so we don't need to do this anymore, but for now you must predict all of the traps.

[wdng]

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

So, once compiler detects "llvm.trap()", shall we set up the trap handler bit in the kernel_code_t? I don't know why we need to add the trap handler bit. Does the bit setting will imply the pretense of the trap handler and otherwise it won't?

[wdng]

1. Fixed format issue.
2. Add test case for llvm.debugtrap.
3. Add non-hsa path trap handler.
4. Confirm that 16 reserved SGPRs are not part of the regular SGPRs.
5. Add trap handler bit set in the amd_kernel_code_t.

[wdng]

vim added some indentations by mistake, removed indentations & empty line.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

276–279 ↗

(On Diff #85971)

This does not need an else

1794 ↗

(On Diff #85971)

const DebugLoc &DL

1986 ↗

(On Diff #85971)

return on next line

lib/Target/AMDGPU/SIMachineFunctionInfo.h

253 ↗

(On Diff #85971)

This should return void and doesn't need HSA in the name

test/CodeGen/AMDGPU/trap.ll

1 ↗

(On Diff #85971)

This should not be checking stderr since the test should pass with HSA

2 ↗

(On Diff #85971)

This needs a not

8 ↗

(On Diff #85971)

Check still missing for the enable_trap_handler bit in kernel_code_t

26 ↗

(On Diff #85971)

GCN should be replaced with ERROR or something like that

30 ↗

(On Diff #85971)

You should add another test which ends in unreachable. As far as I can tell there is no flag to set TrapUnreachable, and we don't enable it now, but this should catch it if that ever breaks

30 ↗

(On Diff #85971)

You also add another test which needs to enable the queue ptr for a different feature

[tony-tye]

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

It seems that the presence of a trap handler is a property of producing a code object to be executed using the HSA environment. Under ROCM all kernels will be executed with a trap handler and that trap handler will use the HSA ABI. Other environments may have different trap handlers, or no trap handler at all, and the code generated for llvm.trap/debugtrap would change accordingly. The bit in amd_kernel_code_t would then be set according to the demands of environment independent of using traps.

Having a trap handler present does cause an extra 16 SGPRs to be allocated (which should be taken into account when determining wave occupancy), but these are in addition to the SGRs used by the generated code (so no need to reserve them or include them in the SGPR count in the amd_kernel_code_t).

[arsenm]

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

Do we still need to make sure we leave 16 unallocated for the implicit use? From the ABI spec it seems clear to not include them in the reported SGPR count, but do we need to ensure reported total + 16 is below the hardware limit?

[tony-tye]

test/CodeGen/AMDGPU/trap.ll

7–8 ↗

(On Diff #85535)

The hardware limit is the maximum non-privileged SGPRs that can be accessed using the instruction register encoding plus the 16 privileged trap registers. The trap temps are only allocated IF a trap handler is enabled, and are only accessible when executing the trap handler (using special instruction register encoding).

So from the point of view of determining the limit of non-privileged SGPRs that can be allocated the presence of a trap handler can be ignored. The fact they are allocated needs to be considered when determining the number of waves that will fit on a CU.

[wdng]

Address code reviews.

[wdng]

lib/Target/AMDGPU/SIISelLowering.cpp

276–279 ↗

(On Diff #85971)

I think we need leave the else here. I assume default setting is Legal, but looks like we need to explicitly specify it otherwise it will throw an error.

test/CodeGen/AMDGPU/trap.ll

30 ↗

(On Diff #85971)

What feature should I test?

30 ↗

(On Diff #85971)

What kinds of instructiosn will trigger the trap instruction?

[tony-tye]

lib/Target/AMDGPU/SIISelLowering.cpp

276–279 ↗

(On Diff #85971)

The else implements the non-HSA expansion that generates an end_prm which is used for graphics.

[kzhuravl]

Can you upload a full diff? Thanks

[wdng]

lib/Target/AMDGPU/SIISelLowering.cpp

276–279 ↗

(On Diff #85971)

yes, so I think we need to keep the else part.

[wdng]

Upload a full diff.

[arsenm]

lib/Target/AMDGPU/SIMachineFunctionInfo.h

265–266 ↗

(On Diff #86133)

Remove Ptr from the names

test/CodeGen/AMDGPU/trap.ll

30 ↗

(On Diff #85971)

You can do an address space cast from LDS to flat or do something with the queue ptr intrinsic

[tony-tye]

lib/Target/AMDGPU/SIISelLowering.cpp

276 ↗

(On Diff #86133)

In discussion with @kzhuravl the suggestion was to use a subtarget query to determine the trap abi. Currently there are two ABIs, one for HSA and one for graphics, but in the future there could be others.

lib/Target/AMDGPU/SIMachineFunctionInfo.h

253 ↗

(On Diff #86133)

As mentioned in another comment, I think determining if there is a trap handler should be determined from the environment part of the triple. Check with @kzhuravl where the query should be put.

[kzhuravl]

As @tony-tye mentioned trap handler is more of an environment property (see https://reviews.llvm.org/D26010#inline-251754), so it should be moved into AMDGPUSubtarget or SISubtarget, and we should be setting trap handler bit in pgm rsrc 2 based on the subtarget query.

In order to allow extensibility in the future the suggestion is to introduce an enumeration with available trap handler ABIs. Currently there will be only 2, something along the lines:

enum TrapHandlerAbi {
  TrapHandlerAbiNone = 0,
  TrapHandlerAbiHsa = 1
}

Also, introduce a subtarget query that returns trap handler abi:

TrapHandlerAbi() {
  if (isAmdHsaOs)
    return TrapHandlerAbiHsa;
  return TrapHandlerAbiNone;
}

Every ISD::TRAP is going to be lowered into S_TRAP_PSEUDO regardless of the trap handler ABI.
In order to expand S_TRAP_PSEUDO, we will have to query what kind of trap handler ABI does the subtarget have. If it is an HSA, then move 1 into v0, and queue ptr into s[0:1]. If trap ABI is none, then lower it to s_engpgm.

@tony-tye did I miss anything? @arsenm, @wdng what do you think?

Thanks

lib/Target/AMDGPU/AMDKernelCodeT.h

200–202 ↗

(On Diff #86133)

This is not right. Trap handler bit is located in amd_compute_pgm_rsrc2_t with the field name enable_trap_handler.

Refer to:

• https://github.com/RadeonOpenCompute/ROCR-Runtime/blob/master/src/inc/amd_hsa_kernel_code.h#L114
• https://github.com/RadeonOpenCompute/ROCm-ComputeABI-Doc/blob/master/AMDGPU-ABI.md#compute-shader-program-settings-2-amd_compute_pgm_rsrc2_t

lib/Target/AMDGPU/SIISelLowering.cpp

278–279 ↗

(On Diff #86133)

If we go with the approach in my latest comment, custom lowering won't be needed.

1798–1809 ↗

(On Diff #86133)

If we go with the approach in my latest comment, this should switch on ABI type, for HSA ABI this code will be used. For "none" ABIs s_engpgm should be used (without error or warning).

2461–2477 ↗

(On Diff #86133)

If we go with the approach in my latest comment, this will be gone.

lib/Target/AMDGPU/SIMachineFunctionInfo.h

253 ↗

(On Diff #86133)

If we go with the approach in my latest comment, this will be moved to subtarget class.

lib/Target/AMDGPU/Utils/AMDKernelCodeTInfo.h

90 ↗

(On Diff #86133)

enable_trap_handler should be here (see comment with links).

115 ↗

(On Diff #86133)

There is no is_trap_handler_supported in CODEPROP (see comment with links).

[kzhuravl]

Also, we need to update waves-per-eu calculations.

[wdng]

In D26010#660492, @kzhuravl wrote:

Also, we need to update waves-per-eu calculations.

What kinds of changes should we made for the waves-per-eu?

[wdng]

1. Restructure code to make it extensible based on code reviews.
2. Move trap handler bit in amd_compute_pgm_rsrc2_t.

[wdng]

1. updated patch with complete & full diff
2. Restructure code to make it extensible based on code reviews.
3. Move trap handler bit in amd_compute_pgm_rsrc2_t.

[arsenm]

lib/Target/AMDGPU/AMDGPUSubtarget.h

100 ↗

(On Diff #86456)

Comment unnecessary, also nothing HSA specific here

267 ↗

(On Diff #86456)

Return enum type

267–272 ↗

(On Diff #86456)

Badly formatted. return ternary operator

lib/Target/AMDGPU/SIISelLowering.cpp

1792 ↗

(On Diff #86456)

Compare to enum value

1804–1808 ↗

(On Diff #86456)

Formatting of previous code needs to be fixed

1809–1810 ↗

(On Diff #86456)

Else on previous line. Instead of else you can early return on the error path

1816–1817 ↗

(On Diff #86456)

Badly formatted. You already have MF and get the Function* above, so you don't need double getParent

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

136–137 ↗

(On Diff #86456)

This should be removed. The trap handler should be set based on a subtarget feature

[wdng]

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

136–137 ↗

(On Diff #86456)

S_00B84C_TRAP_HANDLER(MFI->hasTrapHandler()) needs to know whether trap handler has been set. Where should we the based on the sub- target feature?

[kzhuravl]

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

136–137 ↗

(On Diff #86456)

Set enable_trap_handler bit if Subtarget->getTrapHandlerAbi() != TrapHandlerAbiNone?

lib/Target/AMDGPU/SIMachineFunctionInfo.h

154 ↗

(On Diff #86456)

not needed.

[kzhuravl]

lib/Target/AMDGPU/SIISelLowering.cpp

1806 ↗

(On Diff #86456)

@tony-tye, do we want to use other code for s_trap for llvm.debugtrap?

[tony-tye]

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

641 ↗

(On Diff #86456)

Should this be querying the TrapHandlerAbi using getTrapHandlerAbi() from the subtarget and setting to TRUE if not equal to TrapHandlerAbiNone ?

lib/Target/AMDGPU/AMDGPUSubtarget.h

101 ↗

(On Diff #86456)

Should this be deleted now as there is getTrapHandlerAbi() instead?

lib/Target/AMDGPU/SIISelLowering.cpp

1806 ↗

(On Diff #86456)

Should the 0x1 constant be a named enumeration?

Should we use a different values for llvm.trap than for llvm.debugtrap? If so the handling of llvm.debugtrap should not be converted to llvm.trap for AMDGPU.

Have we checked with teh HSA Runtime to see what codes the HSA trap handler is expecting?

1811–1818 ↗

(On Diff #86456)

Should we be emitting any diagnostic here? If there is no trap handler then doesn't that imply that the environment wants an S_ENDPGM to be generated instead? That is not an error, it is that the environment demands as the implementation. For example, that is what the graphics environment wants.

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

67 ↗

(On Diff #86456)

Delete too?

136–137 ↗

(On Diff #86456)

Delete too?

lib/Target/AMDGPU/SIMachineFunctionInfo.h

154 ↗

(On Diff #86456)

Should this be deleted? See above.

261–263 ↗

(On Diff #86456)

Delete as now we have the trap ABI query instead.

[tony-tye]

Also need to use the trap handler ABI query to see if there is a trap handler, and if there is add the TRAP_HANDLER_SGPR_COUNT to the number of SGPRs budgeted for the wave in determining the number of waves per EU calculation. TRAP_HANDLER_SGPR_COUNT is 16 for GFX6 onwards.

[wdng]

Address code reviews.

[arsenm]

lib/Target/AMDGPU/AMDGPUSubtarget.h

266–269 ↗

(On Diff #86841)

Still using return after else. Should be return ternary operator

328–330 ↗

(On Diff #86841)

This should be a subtarget feature. Returning this is still a constant based purely on the triple. See FeatureUnalignedBufferAccess for an example

test/CodeGen/AMDGPU/trap.ll

1 ↗

(On Diff #86841)

You should have a run line with no subtarget features enabled, and one each explicitly enabling and disabling the trap handler subtarget feature

[wdng]

lib/Target/AMDGPU/SIISelLowering.cpp

1792 ↗

(On Diff #86456)

Say, if we enable trap handler via subtarget feature, should we need to change the if here?

[wdng]

We enable trap handler support once "-mtriple=amdgcn--amdhsa" is specified. So use the following code to enable trap handler.

bool isTrapHandlerEnabled() const {
  return getTrapHandlerAbi() == TrapHandlerAbiHsa;
}

Not sure why we need to add a subtarget feature. Say if we replace code 328-330 using subtarget feature, do we want to enable trap handler by using both "-mtriple=amdgcn--amdhsa" and "-mattr=+enable-trap-handler"?

def FeatureTrapHandler: SubtargetFeature<"enable-trap-handler",
  "EnableTrapHandler",
  "true",
  "Enable trap handler support"
>;

At SIISelLowering.cpp we enable trap handler based on "mtriple=amdgcn--amdhsa", shall we change to based on the subtarget feature input?

if (Subtarget->getTrapHandlerAbi() == SISubtarget::TrapHandlerAbiHsa) {
 . . . 
}

What do you think?

[kzhuravl]

I do not see updates to waves-per-eu calculations for SGPRs. Did you upload the correct diff?

[wdng]

In D26010#665171, @kzhuravl wrote:

I do not see updates to waves-per-eu calculations for SGPRs. Did you upload the correct diff?

Yes, I did. Introduced TRAP_HANDLER_SGPR_COUNT and add it into ExtraSGPRs.

[tony-tye]

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

482–484 ↗

(On Diff #86841)

I do not think enabling the trap handler reduces the number of available SGPRs. The hardware allocates and provides access to theses "in addition" to the regular SGPRs. If this is reserving registers then it should be deleted.

[tony-tye]

lib/Target/AMDGPU/AMDGPUSubtarget.h

73 ↗

(On Diff #86841)

Add enumeration for S_TRAP codes:

enum TrapCode {

TrapCodeLLVMTrap = 1,
TrapCodeLLVMDebugTrap = 2

};

328–330 ↗

(On Diff #86841)

Could you explain this more? The trap handler being present is determined by the environment part of the triple. Currently only the HSA environment uses a trap handler.

329 ↗

(On Diff #86841)

In order to allow additional trap handler ABIs this should be:
getTrapHandlerAbi() != TrapHandlerAbiNone

508 ↗

(On Diff #86841)

I do not see this being used anywhere. I think it should be used in the waves_per_eu calculation.

lib/Target/AMDGPU/SIISelLowering.cpp

1806 ↗

(On Diff #86456)

I checked with the HSA Runtime and currently the trap code is not being consulted. So I propose adding an enum for the codes being used for s_trap and use the value here. It would be better to different code for llvm.trap and llvm.debugtrap. Would that require S_TRAP_PSEUDO to have an immediate operand that is set to the trap code used in the S_TRAP instruction.

test/CodeGen/AMDGPU/trap.ll

21 ↗

(On Diff #86841)

Would like it to be:
; HSA: s_trap 2

[wdng]

Address code reviews.

[arsenm]

lib/Target/AMDGPU/AMDGPU.td

70 ↗

(On Diff #86981)

Remove the word enable-

[wdng]

1. Separate llvm.debugtrap and llvm.trap.
2. Decrease 16 from available SGPRs once trap handler is enabled.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

1824 ↗

(On Diff #87286)

Can we have an enum for these values somewhere instead of hard coding 1?

1833–1838 ↗

(On Diff #87286)

This duplicates nearly the entire other switch case. They should be consolidated based on the immediate argument and handled with the single pseudo

lib/Target/AMDGPU/SIInstructions.td

117 ↗

(On Diff #87286)

This conflicts with the VPseudoInst type. You should remove this and change to SPseudoInstSI

121–127 ↗

(On Diff #87286)

You do not need a second pseudo. You should just add an operand to the other pseudo which is just mimicking the operands of the physical instruction.

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87286)

Variable naming convention

test/CodeGen/AMDGPU/trap.ll

1 ↗

(On Diff #87286)

Having FUNC and HSA-FUNC doesn't make sense. Replace these both with just GCN

2 ↗

(On Diff #87286)

Missing a run line with mesa triple

11–13 ↗

(On Diff #87286)

HSA:

20–21 ↗

(On Diff #87286)

The check prefix names should be like HSA-TRAP, HSA-NOTRAP, MESA-TRAP, MESA-NOTRAP

[wdng]

Address code reviews.

[arsenm]

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1034–1045 ↗

(On Diff #87324)

You should not and do not need to touch generic code

lib/Target/AMDGPU/SIISelLowering.cpp

276 ↗

(On Diff #87324)

DEBUGTRAP should also be legal, you are handling the differences in the selection pattern and custom inserter

lib/Target/AMDGPU/SIInstructions.td

114 ↗

(On Diff #87324)

You should use i16imm for the operand type to match the instruction (not that it matters much) and name it the same too (which matters more): ( ins i16imm:$simm16)

395 ↗

(On Diff #87324)

Can you define pseudo-enums for these constants (see for example SRCMODS)

test/CodeGen/AMDGPU/trap.ll

1–2 ↗

(On Diff #87324)

GCN and HSA are not alternative check prefixes. It doesn't make sense to have it on one of these but not the other. HSA-TRAP for the disabled trap handler feature is broken

3–4 ↗

(On Diff #87324)

These should both explicitly use the mesa triple, and enable/disable the trap handler in each. The ones which should error are also missing the not.

25–26 ↗

(On Diff #87324)

These check lines should not be able to coexist

[wdng]

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1034–1045 ↗

(On Diff #87324)

I accidentally touched those lines and have reverted them, thanks!

lib/Target/AMDGPU/SIISelLowering.cpp

276 ↗

(On Diff #87324)

Yes, I did set DEBUGTRAP to Legal for next pattern matching at isel.

test/CodeGen/AMDGPU/trap.ll

3–4 ↗

(On Diff #87324)

As per our discussion with @tony-tye last week, looks like we want to issue warning instead of error, correct?

[wdng]

Address code reviews.

[tony-tye]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87529)

Is this the right place to useSISubtarget::TRAP_HANDLER_SGPR_COUNT? The presence of a trap handler does not reduce the number of SGPRs addressable for allocation by the register allocator. It only affects the wave occupancy calculation (I did not see any change to that code).

[wdng]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87529)

Yes, this is the right place. -mtriple=amdgcn--amdhsa defaults trap handler feature, so ST.isTrapHandlerEnabled() is true, then 16 SGPRs will be deduced otherwise 0 SGPRs will be deducted. @kzhuravl What do you think?

[arsenm]

lib/Target/AMDGPU/AMDKernelCodeT.h

198 ↗

(On Diff #87529)

This change should be fully reverted

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87529)

I don't think so. This is the allocated number

test/CodeGen/AMDGPU/trap.ll

5–9 ↗

(On Diff #87529)

You seem to have removed the HSA enable/disable and do mesa twice?

21 ↗

(On Diff #87529)

You should also check for the rsrc2 bit is set

[wdng]

test/CodeGen/AMDGPU/trap.ll

5–9 ↗

(On Diff #87529)

I also do HSA enable/disable twice:
; enable HSA trap handler
; RUN: llc -mtriple=amdgcn--amdhsa -mattr=+trap-handler -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=HSA-TRAP %s
; disable HSA trap handler
; RUN: llc -mtriple=amdgcn--amdhsa -mattr=-trap-handler -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NO-HSA-TRAP %s
; disable HSA trap handler to catch warning for llvm.debugtrap since llvm.trap doesn't issue any warnings
; RUN: llc -mtriple=amdgcn--amdhsa -mattr=-trap-handler -verify-machineinstrs < %s 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING %s

[wdng]

Address code reviews.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

1797 ↗

(On Diff #87587)

Should have enum (probably in SIDefines.h) for the values put in

[wdng]

lib/Target/AMDGPU/SIISelLowering.cpp

1797 ↗

(On Diff #87587)

This probably is not an enum. It looks like an interface with HSA is that we need to set v0 to 1:

v0 <- 1

[kzhuravl]

lib/Target/AMDGPU/SIISelLowering.cpp

1797 ↗

(On Diff #87587)

I think defining it in SIDefines.h for now is OK. I am working on some restructuring to shared header files, and will include it in the restructuring.

[kzhuravl]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87529)

I think this should be moved to getMaxNumSGPRs. We still want to report the correct number of addressable SGPRs regardless of trap handler.

[wdng]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87529)

I think in terms of functionality it's the same no matter whether we move the code into getMaxNumSGPRs or not. As getMaxNumSGPRs still invokes this function to calculate MaxNumSGPRs and MaxNumAddressableSGPRs. What do you think?

[kzhuravl]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87529)

Not really. We need to record addressable number of SGPRs in the metadata for tools such as debugger and profiler. The addressable number of SGPRs should be recorded correctly regardless of trap handler (102 for 8+, 104 for everything else).

https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AMDGPU/MCTargetDesc/AMDGPURuntimeMD.cpp#L422

[wdng]

1. Add rsrc2 trap handler bit check in lit tests.
2. Moved the trap handler SGPRs calculation into getMaxNumSGPRs
3. Add a trap operand enum for the time being to replace a constant value 1.

[kzhuravl]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1166 ↗

(On Diff #87529)

Can we drop waves-per-eu calculations from this patch? We need to change few things to math, which can be done as a separate patch.

[wdng]

Drop waves-per-eu calculations.

[arsenm]

test/CodeGen/AMDGPU/trap.ll

1 ↗

(On Diff #87702)

Missing GCN check prefix

12 ↗

(On Diff #87702)

You don't need a check prefix for NO-RSRC2-BIT. That is too specific, the checks should be based on the environment type/options

25 ↗

(On Diff #87702)

This is just the comment printed. You should check the actual register config register value too

[wdng]

Address code reviews.

[tony-tye]

lib/Target/AMDGPU/AMDGPUSubtarget.h

74–82 ↗

(On Diff #87727)

After discussion with the HSA runtime team it was decided to use separate S_TRAP codes for each purpose. The current codes would be:

enum TrapCode {
  TrapCodeBreakpoint = 0,
  TrapCodeLLVMTrap = 1,
  TrapCodeLLVMDebugTrap = 2,
  TrapCodeHSADebugTrap = 3
};

These need documenting in the AMDGPU LLVM Spec page.

For now we do not have an intrinsic for the HSA debugtrap (which takes an argument) so remove the TrapRegValues enum.

lib/Target/AMDGPU/SIISelLowering.cpp

1796–1797 ↗

(On Diff #87727)

Delete this as the traps generated for llvm.trap and llvm.debugtrap will not pass in any value.

[tony-tye]

lib/Target/AMDGPU/SIISelLowering.cpp

1811 ↗

(On Diff #87727)

Delete this as the traps generated for llvm.trap and llvm.debugtrap will not pass in any value.

[tony-tye]

lib/Target/AMDGPU/AMDGPUSubtarget.h

74–82 ↗

(On Diff #87727)

The TRAP handler codes can be defined in a new section of:

http://llvm.org/docs/AMDGPUUsage.html

[wdng]

Added enum for better HSA interface and removed mov instruction for V0 as llvm.trap and llvm.debugtrap will not pass in any value.

[wdng]

Upload correct diff file.

[tony-tye]

lib/Target/AMDGPU/SIISelLowering.cpp

1820 ↗

(On Diff #87764)

Since llvm.debugtrap is not defined as NORETURN then it seems after a warning it should be lowered to a S_NOP not an S_ENDPGM.

llvm.trap is defined as NORETURN so generating S_ENDPGM seems the right choice when there is no trap handler.

[wdng]

Add S_NOP.

[wdng]

Upload full correct diff.

[tony-tye]

lib/Target/AMDGPU/SIISelLowering.cpp

1811 ↗

(On Diff #87817)

Since TrapType is an enum type should this be a switch with a default: that asserts? Currently the code will silently ignore any new trap types that are added (an assert would avoid that).

[wdng]

Address code reviews.

[arsenm]

lib/Target/AMDGPU/AMDGPUSubtarget.cpp

87 ↗

(On Diff #87848)

Sort

lib/Target/AMDGPU/AMDGPUSubtarget.h

106 ↗

(On Diff #87848)

Can you sort this later until after EnableXNACK? I think the unaligned options should stay together

lib/Target/AMDGPU/SIISelLowering.cpp

1813–1815 ↗

(On Diff #87848)

Asserting !the switch case in the switch case is ugly. There's no reason to special case this, just put unreachable in the default case

1826 ↗

(On Diff #87848)

addImm on next line.

We don't really need to emit anything here though

1829–1832 ↗

(On Diff #87848)

Just let it default

1834 ↗

(On Diff #87848)

llvm_unreachable

[tony-tye]

lib/Target/AMDGPU/SIISelLowering.cpp

1826 ↗

(On Diff #87848)

Discussed with @arsenm and leaving as a NOP does give some flexibility asa tool may want to patch these points. Since llvm.debugtrap is not being generally used it does not hurt to have a NOP.

[wdng]

Address code reviews.

[wdng]

Discussed with @tony-tye , keep only 2 CASEs that can happen for the pseudo trap is llvmtrap and llvmdebugtrap in switch block.

[arsenm]

LGTM except for minor issues

lib/Target/AMDGPU/SIISelLowering.cpp

1828 ↗

(On Diff #87863)

Remove dead line

lib/Target/AMDGPU/SIInstructions.td

392 ↗

(On Diff #87863)

Space after :

402 ↗

(On Diff #87863)

Ditto

llvm/trunk/docs/AMDGPUUsage.rst

.. code-block:: nasm
   v_mov_b32 v1, v2 dst_sel:BYTE_0 dst_unused:UNUSED_PRESERVE src0_sel:DWORD
   v_min_u32 v200, v200, v1 dst_sel:WORD_1 dst_unused:UNUSED_PAD src0_sel:BYTE_1 src1_sel:DWORD
   v_sin_f32 v0, v0 dst_unused:UNUSED_PAD src0_sel:WORD_1
   v_fract_f32 v0, |v0| dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:WORD_1
   v_cmpx_le_u32 vcc, v1, v2 src0_sel:BYTE_2 src1_sel:WORD_0
 
 For full list of supported instructions, refer to "Vector ALU instructions".
 
+Trap Handler ABI
+--------------------------
+The Trap Handler suppored is implemented differently based on the host OS. OS
+is obtained from the appropriate element of the target triple HSA OS:
+
+.. code-block:: c++
+  enum TrapHandlerAbi {
+    TrapHandlerAbiNone = 0,
+    TrapHandlerAbiHsa = 1
+  };
+
+  TrapHandlerAbi getTrapHandlerAbi() const {
+    return isAmdHsaOS() ? TrapHandlerAbiHsa : TrapHandlerAbiNone;
+  }
+
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+For HSA OS, a trap handler is always enabled and that the following S_TRAP immediate
+operand codes are supported:
+.. code-block:: c++
+  enum TrapCode {
+    TrapCodeBreakPoint = 0,
+    TrapCodeLLVMTrap = 1,
+    TrapCodeLLVMDebugTrap = 2,
+    TrapCodeHSADebugTrap = 3
+  };
+
+- 0: Used for debugger breakpoint. If debugger is not installed causes dispatch
+to be terminated and its associated queue put into the error state.
+- 1: Used for llvm.trap..queue_ptr is in SGPR0-1. Causes dispatch to be
+terminated and its associated queue put into the error state.
+- 2: Used for llvm.debugtrap. queue_ptr is in SGPR0-1. If debugger not installed
+handled same as llvm.trap.
+- 3: Used for HSA DEBUGTRAP. queue_ptr is in SGPR0-1, the user code is in VGPR0.
+
+Graphics
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+For Graphics, S_ENDPGM is generated for llvm.trap. S_NOP is generated for
+llvm.debugtrap together with a warning that there is no trap handler installed.
+
 HSA Code Object Directives
 --------------------------
 
 AMDGPU ABI defines auxiliary data in output code object. In assembly source,
 one can specify them with assembler directives.
 
 .hsa_code_object_version major, minor
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

llvm/trunk/lib/Target/AMDGPU/AMDGPU.td

 >;
 
 def FeatureUnalignedBufferAccess : SubtargetFeature<"unaligned-buffer-access",
   "UnalignedBufferAccess",
   "true",
   "Support unaligned global loads and stores"
 >;
 
+def FeatureTrapHandler: SubtargetFeature<"trap-handler",
+  "TrapHandler",
+  "true",
+  "Trap handler support"
+>;
+
 def FeatureUnalignedScratchAccess : SubtargetFeature<"unaligned-scratch-access",
   "UnalignedScratchAccess",
   "true",
   "Support unaligned scratch loads and stores"
 >;
 
 // XNACK is disabled if SH_MEM_CONFIG.ADDRESS_MODE = GPUVM on chips that support
 // XNACK. The current default kernel driver setting is:

llvm/trunk/lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp

static const StringRef IntrinsicToAttr[][2] = {
     { "llvm.r600.read.tidig.y", "amdgpu-work-item-id-y" },
     { "llvm.r600.read.tidig.z", "amdgpu-work-item-id-z" }
   };
 
   static const StringRef HSAIntrinsicToAttr[][2] = {
     { "llvm.amdgcn.dispatch.ptr", "amdgpu-dispatch-ptr" },
     { "llvm.amdgcn.queue.ptr", "amdgpu-queue-ptr" },
     { "llvm.amdgcn.dispatch.id", "amdgpu-dispatch-id" },
-  { "llvm.trap", "amdgpu-queue-ptr" }
+    { "llvm.trap", "amdgpu-queue-ptr" },
+    { "llvm.debugtrap", "amdgpu-queue-ptr" }
   };
 
   // TODO: We should not add the attributes if the known compile time workgroup
   // size is 1 for y/z.
 
   // TODO: Intrinsics that require queue ptr.
 
   // We do not need to note the x workitem or workgroup id because they are

llvm/trunk/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

if (STM.getGeneration() >= AMDGPUSubtarget::SOUTHERN_ISLANDS) {
                                     Twine(KernelInfo.DebuggerWavefrontPrivateSegmentOffsetSGPR), false);
         OutStreamer->emitRawComment(" DebuggerPrivateSegmentBufferSGPR: s" +
                                     Twine(KernelInfo.DebuggerPrivateSegmentBufferSGPR), false);
       }
 
       OutStreamer->emitRawComment(" COMPUTE_PGM_RSRC2:USER_SGPR: " +
                                   Twine(G_00B84C_USER_SGPR(KernelInfo.ComputePGMRSrc2)),
                                   false);
+      OutStreamer->emitRawComment(" COMPUTE_PGM_RSRC2:TRAP_HANDLER: " +
+                                  Twine(G_00B84C_TRAP_HANDLER(KernelInfo.ComputePGMRSrc2)),
+                                  false);
       OutStreamer->emitRawComment(" COMPUTE_PGM_RSRC2:TGID_X_EN: " +
                                   Twine(G_00B84C_TGID_X_EN(KernelInfo.ComputePGMRSrc2)),
                                   false);
       OutStreamer->emitRawComment(" COMPUTE_PGM_RSRC2:TGID_Y_EN: " +
                                   Twine(G_00B84C_TGID_Y_EN(KernelInfo.ComputePGMRSrc2)),
                                   false);
       OutStreamer->emitRawComment(" COMPUTE_PGM_RSRC2:TGID_Z_EN: " +
                                   Twine(G_00B84C_TGID_Z_EN(KernelInfo.ComputePGMRSrc2)),
void AMDGPUAsmPrinter::getSIProgramInfo(SIProgramInfo &ProgInfo,
   if (MFI->hasWorkItemIDZ())
     TIDIGCompCnt = 2;
   else if (MFI->hasWorkItemIDY())
     TIDIGCompCnt = 1;
 
   ProgInfo.ComputePGMRSrc2 =
       S_00B84C_SCRATCH_EN(ProgInfo.ScratchBlocks > 0) |
       S_00B84C_USER_SGPR(MFI->getNumUserSGPRs()) |
+      S_00B84C_TRAP_HANDLER(STM.isTrapHandlerEnabled()) |
       S_00B84C_TGID_X_EN(MFI->hasWorkGroupIDX()) |
       S_00B84C_TGID_Y_EN(MFI->hasWorkGroupIDY()) |
       S_00B84C_TGID_Z_EN(MFI->hasWorkGroupIDZ()) |
       S_00B84C_TG_SIZE_EN(MFI->hasWorkGroupInfo()) |
       S_00B84C_TIDIG_COMP_CNT(TIDIGCompCnt) |
       S_00B84C_EXCP_EN_MSB(0) |
       S_00B84C_LDS_SIZE(ProgInfo.LDSBlocks) |
       S_00B84C_EXCP_EN(0);

llvm/trunk/lib/Target/AMDGPU/AMDGPUSubtarget.h

enum {
     ISAVersion8_0_0,
     ISAVersion8_0_1,
     ISAVersion8_0_2,
     ISAVersion8_0_3,
     ISAVersion8_0_4,
     ISAVersion8_1_0,
   };
 
+  enum TrapHandlerAbi {
+    TrapHandlerAbiNone = 0,
+    TrapHandlerAbiHsa = 1
+  };
+
+  enum TrapCode {
+    TrapCodeBreakPoint = 0,
+    TrapCodeLLVMTrap = 1,
+    TrapCodeLLVMDebugTrap = 2,
+    TrapCodeHSADebugTrap = 3
+  };
+
+  enum TrapRegValues {
+    TrapCodeLLVMTrapRegValue = 1
+  };
+
 protected:
   // Basic subtarget description.
   Triple TargetTriple;
   Generation Gen;
   unsigned IsaVersion;
   unsigned WavefrontSize;
   int LocalMemorySize;
   int LDSBankCount;
   unsigned MaxPrivateElementSize;
 
   // Possibly statically set by tablegen, but may want to be overridden.
   bool FastFMAF32;
   bool HalfRate64Ops;
 
   // Dynamially set bits that enable features.
   bool FP32Denormals;
   bool FP64FP16Denormals;
   bool FPExceptions;
   bool FlatForGlobal;
   bool UnalignedScratchAccess;
   bool UnalignedBufferAccess;
   bool EnableXNACK;
+  bool TrapHandler;
   bool DebuggerInsertNops;
   bool DebuggerReserveRegs;
   bool DebuggerEmitPrologue;
 
   // Used as options.
   bool EnableVGPRSpilling;
   bool EnablePromoteAlloca;
   bool EnableLoadStoreOpt;
public:
   bool hasBORROW() const {
     return (getGeneration() >= EVERGREEN);
   }
 
   bool hasCaymanISA() const {
     return CaymanISA;
   }
 
+  TrapHandlerAbi getTrapHandlerAbi() const {
+    return isAmdHsaOS() ? TrapHandlerAbiHsa : TrapHandlerAbiNone;
+  }
+
   bool isPromoteAllocaEnabled() const {
     return EnablePromoteAlloca;
   }
 
   bool unsafeDSOffsetFoldingEnabled() const {
     return EnableUnsafeDSOffsetFolding;
   }
 
public:
   bool hasUnalignedBufferAccess() const {
     return UnalignedBufferAccess;
   }
 
   bool hasUnalignedScratchAccess() const {
     return UnalignedScratchAccess;
   }
 
+  bool isTrapHandlerEnabled() const {
+    return TrapHandler;
+  }
+
   bool isXNACKEnabled() const {
     return EnableXNACK;
   }
 
   bool hasFlatAddressSpace() const {
     return FlatAddressSpace;
   }
 

llvm/trunk/lib/Target/AMDGPU/AMDGPUSubtarget.cpp

AMDGPUSubtarget::initializeSubtargetDependencies(const Triple &TT,
   // double precision rate, so don't enable by default.
   //
   // We want to be able to turn these off, but making this a subtarget feature
   // for SI has the unhelpful behavior that it unsets everything else if you
   // disable it.
 
   SmallString<256> FullFS("+promote-alloca,+fp64-fp16-denormals,+load-store-opt,");
   if (isAmdHsaOS()) // Turn on FlatForGlobal for HSA.
-    FullFS += "+flat-for-global,+unaligned-buffer-access,";
+    FullFS += "+flat-for-global,+unaligned-buffer-access,+trap-handler,";
 
   FullFS += FS;
 
   ParseSubtargetFeatures(GPU, FullFS);
 
   // Unless +-flat-for-global is specified, turn on FlatForGlobal for all OS-es
   // on VI and newer hardware to avoid assertion failures due to missing ADDR64
   // variants of MUBUF instructions.
: AMDGPUGenSubtargetInfo(TT, GPU, FS),
     FP32Denormals(false),
     FP64FP16Denormals(false),
     FPExceptions(false),
     FlatForGlobal(false),
     UnalignedScratchAccess(false),
     UnalignedBufferAccess(false),
 
     EnableXNACK(false),
+    TrapHandler(false),
     DebuggerInsertNops(false),
     DebuggerReserveRegs(false),
     DebuggerEmitPrologue(false),
 
     EnableVGPRSpilling(false),
     EnablePromoteAlloca(false),
     EnableLoadStoreOpt(false),
     EnableUnsafeDSOffsetFolding(false),

llvm/trunk/lib/Target/AMDGPU/SIDefines.h

 
 #define R_00B84C_COMPUTE_PGM_RSRC2                                      0x00B84C
 #define   S_00B84C_SCRATCH_EN(x)                                      (((x) & 0x1) << 0)
 #define   G_00B84C_SCRATCH_EN(x)                                      (((x) >> 0) & 0x1)
 #define   C_00B84C_SCRATCH_EN                                         0xFFFFFFFE
 #define   S_00B84C_USER_SGPR(x)                                       (((x) & 0x1F) << 1)
 #define   G_00B84C_USER_SGPR(x)                                       (((x) >> 1) & 0x1F)
 #define   C_00B84C_USER_SGPR                                          0xFFFFFFC1
+#define   S_00B84C_TRAP_HANDLER(x)                                    (((x) & 0x1) << 6)
+#define   G_00B84C_TRAP_HANDLER(x)                                    (((x) >> 6) & 0x1)
+#define   C_00B84C_TRAP_HANDLER                                       0xFFFFFFBF
 #define   S_00B84C_TGID_X_EN(x)                                       (((x) & 0x1) << 7)
 #define   G_00B84C_TGID_X_EN(x)                                       (((x) >> 7) & 0x1)
 #define   C_00B84C_TGID_X_EN                                          0xFFFFFF7F
 #define   S_00B84C_TGID_Y_EN(x)                                       (((x) & 0x1) << 8)
 #define   G_00B84C_TGID_Y_EN(x)                                       (((x) >> 8) & 0x1)
 #define   C_00B84C_TGID_Y_EN                                          0xFFFFFEFF
 #define   S_00B84C_TGID_Z_EN(x)                                       (((x) & 0x1) << 9)
 #define   G_00B84C_TGID_Z_EN(x)                                       (((x) >> 9) & 0x1)

 #define R_00B860_COMPUTE_TMPRING_SIZE                                   0x00B860
 #define   S_00B860_WAVESIZE(x)                                        (((x) & 0x1FFF) << 12)
 
 #define R_0286E8_SPI_TMPRING_SIZE                                       0x0286E8
 #define   S_0286E8_WAVESIZE(x)                                        (((x) & 0x1FFF) << 12)
 
 #define R_SPILLED_SGPRS         0x4
 #define R_SPILLED_VGPRS         0x8
-
 } // End namespace llvm
 
 #endif

llvm/trunk/lib/Target/AMDGPU/SIISelLowering.cpp

SITargetLowering::SITargetLowering(const TargetMachine &TM,
   }
 
   setOperationAction(ISD::BSWAP, MVT::i32, Legal);
   setOperationAction(ISD::BITREVERSE, MVT::i32, Legal);
 
   // On SI this is s_memtime and s_memrealtime on VI.
   setOperationAction(ISD::READCYCLECOUNTER, MVT::i64, Legal);
   setOperationAction(ISD::TRAP, MVT::Other, Legal);
+  setOperationAction(ISD::DEBUGTRAP, MVT::Other, Legal);
 
   setOperationAction(ISD::FMINNUM, MVT::f64, Legal);
   setOperationAction(ISD::FMAXNUM, MVT::f64, Legal);
 
   if (Subtarget->getGeneration() >= SISubtarget::SEA_ISLANDS) {
     setOperationAction(ISD::FTRUNC, MVT::f64, Legal);
     setOperationAction(ISD::FCEIL, MVT::f64, Legal);
     setOperationAction(ISD::FRINT, MVT::f64, Legal);
if (MI.mayLoad())
       Flags |= MachineMemOperand::MOLoad;
 
     auto MMO = MF->getMachineMemOperand(PtrInfo, Flags, 0, 0);
     MI.addMemOperand(*MF, MMO);
     return BB;
   }
 
   switch (MI.getOpcode()) {
-   case AMDGPU::S_TRAP_PSEUDO: {
-  DebugLoc DL = MI.getDebugLoc();
-  BuildMI(*BB, MI, DL, TII->get(AMDGPU::V_MOV_B32_e32), AMDGPU::VGPR0)
-     .addImm(1);
+    const DebugLoc &DL = MI.getDebugLoc();
+    const int TrapType = MI.getOperand(0).getImm(); 
+
+    if (Subtarget->getTrapHandlerAbi() == SISubtarget::TrapHandlerAbiHsa &&
+        Subtarget->isTrapHandlerEnabled()) {
 
       MachineFunction *MF = BB->getParent();
       SIMachineFunctionInfo *Info = MF->getInfo<SIMachineFunctionInfo>();
       unsigned UserSGPR = Info->getQueuePtrUserSGPR();
       assert(UserSGPR != AMDGPU::NoRegister);
 
       if (!BB->isLiveIn(UserSGPR))
         BB->addLiveIn(UserSGPR);
 
       BuildMI(*BB, MI, DL, TII->get(AMDGPU::COPY), AMDGPU::SGPR0_SGPR1)
         .addReg(UserSGPR);
-    BuildMI(*BB, MI, DL, TII->get(AMDGPU::S_TRAP)).addImm(0x1)
-     .addReg(AMDGPU::VGPR0, RegState::Implicit)
+      BuildMI(*BB, MI, DL, TII->get(AMDGPU::S_TRAP))
+        .addImm(TrapType)
         .addReg(AMDGPU::SGPR0_SGPR1, RegState::Implicit);
+    } else {
+      switch (TrapType) {   
+      case SISubtarget::TrapCodeLLVMTrap: 
+        BuildMI(*BB, MI, DL, TII->get(AMDGPU::S_ENDPGM));
+        break;
+      case SISubtarget::TrapCodeLLVMDebugTrap: {
+        DiagnosticInfoUnsupported NoTrap(*MF->getFunction(),
+                                         "debugtrap handler not supported",
+                                         DL,
+                                         DS_Warning);
+        LLVMContext &C = MF->getFunction()->getContext();     
+        C.diagnose(NoTrap);
+        BuildMI(*BB, MI, DL, TII->get(AMDGPU::S_NOP))
+          .addImm(0);
+        break;
+      }
+      default:
+        llvm_unreachable("unsupported trap handler type!");
+      }
+    }
 
     MI.eraseFromParent();
     return BB;
   }
 
   case AMDGPU::SI_INIT_M0:
     BuildMI(*BB, MI.getIterator(), MI.getDebugLoc(),
             TII->get(AMDGPU::S_MOV_B32), AMDGPU::M0)

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.td

 def DSTCLAMP {
   int NONE = 0;
 }
 
 def DSTOMOD {
   int NONE = 0;
 }
 
+def TRAPTYPE {
+  int LLVM_TRAP = 1;
+  int LLVM_DEBUG_TRAP = 2;
+}
+
 //===----------------------------------------------------------------------===//
 //
 // SI Instruction multiclass helpers.
 //
 // Instructions with _32 take 32-bit operands.
 // Instructions with _64 take 64-bit operands.
 //
 // VOP_* instructions can use either a 32-bit or 64-bit encoding.  The 32-bit

llvm/trunk/lib/Target/AMDGPU/SIInstructions.td

 }
 
 // 64-bit vector move instruction.  This is mainly used by the SIFoldOperands
 // pass to enable folding of inline immediates.
 def V_MOV_B64_PSEUDO : VPseudoInstSI <(outs VReg_64:$vdst),
                                       (ins VSrc_b64:$src0)>;
 } // End let hasSideEffects = 0, mayLoad = 0, mayStore = 0, Uses = [EXEC]
 
-def S_TRAP_PSEUDO : VPseudoInstSI <(outs), (ins),
-  [(trap)]> {
+def S_TRAP_PSEUDO : SPseudoInstSI <(outs), (ins i16imm:$simm16)> {
   let hasSideEffects = 1;
   let SALU = 1;
   let usesCustomInserter = 1;
 }
 
 let usesCustomInserter = 1, SALU = 1 in {
 def GET_GROUPSTATICSIZE : PseudoInstSI <(outs SReg_32:$sdst), (ins),
   [(set SReg_32:$sdst, (int_amdgcn_groupstaticsize))]>;
def SI_PC_ADD_REL_OFFSET : SPseudoInstSI <
   [(set SReg_64:$dst,
    (i64 (SIpc_add_rel_offset (tglobaladdr:$ptr_lo), (tglobaladdr:$ptr_hi))))]> {
   let Defs = [SCC];
 }
 
 } // End SubtargetPredicate = isGCN
 
 let Predicates = [isGCN] in {
+def : Pat<
+  (trap),
+  (S_TRAP_PSEUDO TRAPTYPE.LLVM_TRAP)
+>;
+
+def : Pat<
+  (debugtrap),
+  (S_TRAP_PSEUDO TRAPTYPE.LLVM_DEBUG_TRAP)
+>;
 
 def : Pat<
   (int_amdgcn_else i64:$src, bb:$target),
   (SI_ELSE $src, $target, 0)
 >;
 
 def : Pat <
   (int_AMDGPU_kilp),

llvm/trunk/lib/Target/AMDGPU/Utils/AMDKernelCodeTInfo.h

 COMPPGM1(priv,                            compute_pgm_rsrc1_priv,           PRIV),
 COMPPGM1(enable_dx10_clamp,               compute_pgm_rsrc1_dx10_clamp,     DX10_CLAMP),
 COMPPGM1(debug_mode,                      compute_pgm_rsrc1_debug_mode,     DEBUG_MODE),
 COMPPGM1(enable_ieee_mode,                compute_pgm_rsrc1_ieee_mode,      IEEE_MODE),
 // TODO: bulky
 // TODO: cdbg_user
 COMPPGM2(enable_sgpr_private_segment_wave_byte_offset, compute_pgm_rsrc2_scratch_en, SCRATCH_EN),
 COMPPGM2(user_sgpr_count,                 compute_pgm_rsrc2_user_sgpr,      USER_SGPR),
-// TODO: enable_trap_handler
+COMPPGM2(enable_trap_handler,             compute_pgm_rsrc2_trap_handler,   TRAP_HANDLER),
 COMPPGM2(enable_sgpr_workgroup_id_x,      compute_pgm_rsrc2_tgid_x_en,      TGID_X_EN),
 COMPPGM2(enable_sgpr_workgroup_id_y,      compute_pgm_rsrc2_tgid_y_en,      TGID_Y_EN),
 COMPPGM2(enable_sgpr_workgroup_id_z,      compute_pgm_rsrc2_tgid_z_en,      TGID_Z_EN),
 COMPPGM2(enable_sgpr_workgroup_info,      compute_pgm_rsrc2_tg_size_en,     TG_SIZE_EN),
 COMPPGM2(enable_vgpr_workitem_id,         compute_pgm_rsrc2_tidig_comp_cnt, TIDIG_COMP_CNT),
 COMPPGM2(enable_exception_msb,            compute_pgm_rsrc2_excp_en_msb,    EXCP_EN_MSB), // TODO: split enable_exception_msb
 COMPPGM2(granulated_lds_size,             compute_pgm_rsrc2_lds_size,       LDS_SIZE),
 COMPPGM2(enable_exception,                compute_pgm_rsrc2_excp_en,        EXCP_EN), // TODO: split enable_exception

llvm/trunk/test/CodeGen/AMDGPU/fneg-fabs.f16.ll

 ; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=kaveri -verify-machineinstrs < %s | FileCheck -check-prefix=CI -check-prefix=FUNC %s
 ; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=tonga -verify-machineinstrs < %s | FileCheck -check-prefix=VI -check-prefix=FUNC %s
 
 ; GCN-LABEL: {{^}}fneg_fabs_fadd_f16:
 ; CI: v_cvt_f32_f16_e32
 ; CI: v_cvt_f32_f16_e64 [[CVT_ABS_X:v[0-9]+]], |v{{[0-9]+}}|
 ; CI: v_subrev_f32_e32 v{{[0-9]+}}, [[CVT_ABS_X]], v{{[0-9]+}}
 
-; VI-NOT: and
+; VI-NOT: _and
 ; VI: v_sub_f16_e64 {{v[0-9]+}}, {{v[0-9]+}}, |{{v[0-9]+}}|
 define void @fneg_fabs_fadd_f16(half addrspace(1)* %out, half %x, half %y) {
   %fabs = call half @llvm.fabs.f16(half %x)
   %fsub = fsub half -0.000000e+00, %fabs
   %fadd = fadd half %y, %fsub
   store half %fadd, half addrspace(1)* %out, align 2
   ret void
 }
 
 ; GCN-LABEL: {{^}}fneg_fabs_fmul_f16:
 ; CI-DAG: v_cvt_f32_f16_e32
 ; CI-DAG: v_cvt_f32_f16_e64 [[CVT_NEG_ABS_X:v[0-9]+]], -|{{v[0-9]+}}|
 ; CI: v_mul_f32_e32 {{v[0-9]+}}, [[CVT_NEG_ABS_X]], {{v[0-9]+}}
 ; CI: v_cvt_f16_f32_e32
 
-; VI-NOT: and
+; VI-NOT: _and
 ; VI: v_mul_f16_e64 [[MUL:v[0-9]+]], {{v[0-9]+}}, -|{{v[0-9]+}}|
 ; VI-NOT: [[MUL]]
 ; VI: flat_store_short v{{\[[0-9]+:[0-9]+\]}}, [[MUL]]
 define void @fneg_fabs_fmul_f16(half addrspace(1)* %out, half %x, half %y) {
   %fabs = call half @llvm.fabs.f16(half %x)
   %fsub = fsub half -0.000000e+00, %fabs
   %fmul = fmul half %y, %fsub
   store half %fmul, half addrspace(1)* %out, align 2

llvm/trunk/test/CodeGen/AMDGPU/trap.ll

-; RUN: llc -mtriple=amdgcn--amdhsa -verify-machineinstrs < %s 2>&1 | FileCheck -check-prefix=GCN %s
+; RUN: llc -mtriple=amdgcn--amdhsa -verify-machineinstrs < %s | FileCheck -check-prefix=HSA-TRAP %s
+
+; RUN: llc -mtriple=amdgcn--amdhsa -mattr=+trap-handler -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=HSA-TRAP %s
+; RUN: llc -mtriple=amdgcn--amdhsa -mattr=-trap-handler -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NO-HSA-TRAP %s
+; RUN: llc -mtriple=amdgcn--amdhsa -mattr=-trap-handler -verify-machineinstrs < %s 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING %s
+
+; enable trap handler feature
+; RUN: llc -mtriple=amdgcn-unknown-mesa3d -mattr=+trap-handler -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NO-MESA-TRAP -check-prefix=TRAP-BIT -check-prefix=MESA-TRAP %s
+; RUN: llc -mtriple=amdgcn-unknown-mesa3d -mattr=+trap-handler -verify-machineinstrs < %s 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING -check-prefix=TRAP-BIT %s
+
+; disable trap handler feature
+; RUN: llc -mtriple=amdgcn-unknown-mesa3d -mattr=-trap-handler -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NO-MESA-TRAP -check-prefix=NO-TRAP-BIT -check-prefix=NOMESA-TRAP %s
+; RUN: llc -mtriple=amdgcn-unknown-mesa3d -mattr=-trap-handler -verify-machineinstrs < %s 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING -check-prefix=NO-TRAP-BIT %s
+
+; RUN: llc -march=amdgcn -verify-machineinstrs < %s 2>&1 | FileCheck -check-prefix=GCN -check-prefix=GCN-WARNING %s
 
 declare void @llvm.trap() #0
+declare void @llvm.debugtrap() #0
+
+; MESA-TRAP: .section .AMDGPU.config
+; MESA-TRAP:  .long   47180
+; MESA-TRAP-NEXT: .long   208
+
+; NOMESA-TRAP: .section .AMDGPU.config
+; NOMESA-TRAP:  .long   47180
+; NOMESA-TRAP-NEXT: .long   144
+
+; GCN-LABEL: {{^}}hsa_trap:
+; HSA-TRAP: enable_trap_handler = 1
+; HSA-TRAP: s_mov_b64 s[0:1], s[4:5]
+; HSA-TRAP: s_trap 1
+
+; for llvm.trap in hsa path without ABI, direct generate s_endpgm instruction without any warning information
+; NO-HSA-TRAP: enable_trap_handler = 0
+; NO-HSA-TRAP: s_endpgm
+; NO-HSA-TRAP: COMPUTE_PGM_RSRC2:TRAP_HANDLER: 0
+
+; TRAP-BIT: enable_trap_handler = 1
+; NO-TRAP-BIT: enable_trap_handler = 0
+; NO-MESA-TRAP: s_endpgm
+define void @hsa_trap() {
+  call void @llvm.trap()
+  ret void
+}
+
+; MESA-TRAP: .section .AMDGPU.config
+; MESA-TRAP:  .long   47180
+; MESA-TRAP-NEXT: .long   208
+
+; NOMESA-TRAP: .section .AMDGPU.config
+; NOMESA-TRAP:  .long   47180
+; NOMESA-TRAP-NEXT: .long   144
+
+; GCN-WARNING: warning: <unknown>:0:0: in function hsa_debugtrap void (): debugtrap handler not supported
+; GCN-LABEL: {{^}}hsa_debugtrap:
+; HSA-TRAP: enable_trap_handler = 1
+; HSA-TRAP: s_mov_b64 s[0:1], s[4:5]
+; HSA-TRAP: s_trap 2
+
+; for llvm.debugtrap in non-hsa path without ABI, generate a warning and a s_endpgm instruction
+; NO-HSA-TRAP: enable_trap_handler = 0
+; NO-HSA-TRAP: s_endpgm
+
+; TRAP-BIT: enable_trap_handler = 1
+; NO-TRAP-BIT: enable_trap_handler = 0
+; NO-MESA-TRAP: s_endpgm
+define void @hsa_debugtrap() {
+  call void @llvm.debugtrap()
+  ret void
+}
 
+; For non-HSA path
 ; GCN-LABEL: {{^}}trap:
-; GCN: v_mov_b32_e32 v0, 1
-; GCN: s_mov_b64 s[0:1], s[4:5]
-; GCN: s_trap 1
+; TRAP-BIT: enable_trap_handler = 1
+; NO-TRAP-BIT: enable_trap_handler = 0
+; NO-HSA-TRAP: s_endpgm
+; NO-MESA-TRAP: s_endpgm
 define void @trap() {
   call void @llvm.trap()
   ret void
 }
 
 attributes #0 = { nounwind noreturn }