[PATCH] D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D78778#2449409 <https://reviews.llvm.org/D78778#2449409>, @MaskRay wrote:

> In D78778#2449379 <https://reviews.llvm.org/D78778#2449379>, @scott.linder wrote:
>
>> In D78778#2449342 <https://reviews.llvm.org/D78778#2449342>, @MaskRay wrote:
>>
>>> I think the thing we cannot get past is: `ExceptionHandling::DwarfCFI` is called exception handling and thus we expect it to be only useful with C++ (or other language) exceptions. However, the practice is that we overload it for non-exception .eh_frame/.ARM_extab/.ARM_exidx generation as well. So, adding a behavior to `ExceptionHandling::None` and introducing `SupportsDebugUnwindInformation` just seem to bring more complexity to me.
>>>
>>> If `ExceptionHandling` were called something else, this might be less misleading.
>>
>> Right, I am open to suggestions, but it also just mirrors the implementation in that the code for doing the debug-only, no-exceptions unwind information is still contained in `EHStreamer`/`DwarfCFIException`. Those could also be renamed, but then most of the code in them is irrelevant to/complicates the debug case.
>>
>> That's how I ended up at "make a new streamer that is honest about being for debug-only DWARF CFI", and it happened to be nearly empty, so I didn't even have it share code with the exception variants. Maybe instead of an ad-hoc option, though, it could be another `enum DebugUnwindTables { None; DwarfCFI; }`? And rename the `EHStreamer` hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?
>
> Yes, some renaming can help. I believe MCAsmInfo has sufficient states to encode all the needs.
>
>> Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?
>
> Having both UnwindTable and ExceptionHandling enums sounds good. `isCFIMoveForDebugging` (the name confused me quite a bit) probably should be folded into the new representations.

Ok, I can draft up something with the new enum. As for `isCFIMoveForDebugging` I agreed and deleted it in https://reviews.llvm.org/D76519, which I see you've already commented on, thank you!

>> Maybe those aren't present anymore; is anyone aware if that option should affect CodeGen?
>
> I have scanned the call sites of `getExceptionHandlingType`. DwarfEHPrepapre.cpp can replace `resume` with `_Unwind_Resume`. However, if you don't use C++ exceptions, the pass should be a no-op.
> There should be no target-independent pass which can affect codegen (wasm and arm are complex and I cannot ensure they don't affect codegen but those are their target decisions)

Taking a look again, the difference in codegen was due to `TargetPassConfig::addPassesToHandleExceptions` no longer adding the `LowerInvoke` pass, IIRC. I'm not actually sure we rely on this anywhere outside of some tests, but at that point I determined that we shouldn't "lie" about our `ExceptionsType` if it can affect codegen. Adding the separate, mostly-orthogonal enum for unwind information should solve this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778