[PATCH] D76879: [AMDGPU] Begin emitting CFI for AMDGCN

[Scott Linder via Phabricator via llvm-commits]

scott.linder marked 3 inline comments as done.
scott.linder added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/branch-relaxation.ll:257
 ; GCN-NEXT: .Lfunc_end{{[0-9]+}}:
-define amdgpu_kernel void @uniform_unconditional_min_long_forward_branch(i32 addrspace(1)* %arg, i32 %arg1) {
+define amdgpu_kernel void @uniform_unconditional_min_long_forward_branch(i32 addrspace(1)* %arg, i32 %arg1) #0 {
 bb0:
----------------
arsenm wrote:
> Why does it matter in any of these cases?
When we say we are `ExceptionHandling::DwarfCFI` we are saying that we want an `.eh_frame` for any functions which are not `nounwind` (i.e. anything that can throw) and that we want a `.debug_frame` section for anything else that has debug info enabled. We never need the `.eh_frame` case, and these tests are agnostic to it anyway, so it seemed nicer to just update all tests to be `nounwind` than to add CFI noise to them all.

There is currently no way for a target to say "I only ever want CFI for debugging purposes, because I don't support exceptions anyway". I didn't go about adding one because we always mark functions `nounwind` anyway, and if we ever stopped it would likely be because we intend to support EH and would want the `.eh_frame` to start being generated.


================
Comment at: llvm/test/DebugInfo/AMDGPU/cfi.ll:22
+
+attributes #0 = { nounwind "no-frame-pointer-elim"="true" }
+
----------------
arsenm wrote:
> This is the deprecated attribute. This should use frame-pointer=
This test could use a bit of cleanup, I don't think it depends on this attribute anyway. I'll trim it down to just what is actually relevant.


================
Comment at: llvm/test/MC/ELF/AMDGPU/lit.local.cfg:2
+# We have to reset config.unsupported here because the parent directory is
+# predicated on 'X86'.
+config.unsupported = not 'AMDGPU' in config.root.targets
----------------
arsenm wrote:
> Copy paste X86
I think this X86 is actually correct here, the root llvm/test/MC/ELF dir is already predicated (there is no X86 subdir).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76879