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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D78778#2356959 <https://reviews.llvm.org/D78778#2356959>, @dblaikie wrote:

> In D78778#2322163 <https://reviews.llvm.org/D78778#2322163>, @scott.linder wrote:
>
>> In D78778#2301702 <https://reviews.llvm.org/D78778#2301702>, @probinson wrote:
>>
>>> I was not aware that `.eh_frame` needed to be an ALLOC section, although it makes sense now that you say it; thanks for that!
>>>
>>> I don't feel qualified to approve this, but any reviewer will tell you:  Needs a test.
>>
>> I can't seem to find another example of a test of an MCAsmInfo option, at least not without a target which sets it.
>
> Yep, generally LLVM adds use of functionality available from at least one of the tools (llc, etc) in the patch that adds the functionality and tests it through that.
>
>> I suppose a UnitTest which derives a new class from MCAsmInfo could work, but then I wonder why this hasn't been needed for any of the other options.
>>
>> The tests in https://reviews.llvm.org/D76879 do exercise this, and it seemed like it would be easier to review as two separate commits. The current phabricator "stack" doesn't express the dependencies perfect here, I can move it around some to make it more clear.
>
> Generally adding code that's untested in the patch it's added in is unfavorable. Looks like this should be merged with D76879 <https://reviews.llvm.org/D76879> - specifically because the tests should be assessed in the context of the new code and the new code should be assessed in the context of how the tests cover that functionality to ensure good coverage. (if there's a lot of patches between these two patches/bunch of other functionality - then maybe adding a flag to support optionally using this feature, rather than hardcoding/making it permanently on - then the code can be tested without turning on a half-implemented feature, and adding functionality with associated test coverage incrementally, then at the end removing the optional flag support and making it permanent/constantly enabled)

Thank you for the feedback, I don't think I carefully about what would be testable at each point when I laid out how to structure the patch series. I took your advice and merged the two commits that must be present to test the new MCAsmInfo flag.

I need to go through the rest of the series and see if similar changes are needed, but I think this patch is ready for review again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778