[PATCH] D74688: AMDGPU/GlobalISel: Support llvm.trap and llvm.debugtrap intrinsics

Sun Mar 1 20:33:07 PST 2020

t-tye added a comment.

In D74688#1888777 <https://reviews.llvm.org/D74688#1888777>, @hsmhsm wrote:

> In D74688#1888262 <https://reviews.llvm.org/D74688#1888262>, @t-tye wrote:
>
> > I believe that the debug_trap trap handler support no longer needs the queue_ptr to be passed in as it is internally computing it from the dootbell ID available from a GETREG together with a doorbell to queue mapping maintained by the ROCm Runtim that is accessible through the TMB register.
> >
> > So should that change be reflect in this to simplify things? This is does change the ABI so  needs the HSA ABI number to be incremented in the ELF header and AMDGOUUsage documentation updated. However, from the compilers point of view it is not ABI breaking as old code will still work, as it is generating code to set the queue_ptr that is unnecessary.
> >
> > Adding @kzhuravl for ELF ABI version help.
>
>
> Hi Tony,
>
> If you are specifically asking for llvm.debugtrap() intrinsic here, then, we are already taken care of it, and we are not adding queue_ptr in this case, as you can see from the code changes at AMDGPULegalizerInfo.cpp:3602. The queue_ptr related discussions here are only specific to llvm.trap() intrinsic.

Looking at AMDGPULegalizerInfo.cpp:3602 it appears the queue_ptr is still being set up so not sure what you mean that it is already being taken care of. What I describe is true for both llvm.trap() and llvm.debug_trap(). Should the setup of the queu_ptr be removed since it is no longer needed to support either llvm.trap() or llvm.debug_trap()? Doing so would need a change in AMDGPUUsage, a change to the ELF HSA ABI number, and corresponding ROCm loader changes. There may also need to be a plan to support older ROCm releases.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74688/new/

https://reviews.llvm.org/D74688