[PATCH] D84519: [llvm-objdump][AMDGPU] Detect CPU string

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D84519#2195705 <https://reviews.llvm.org/D84519#2195705>, @rochauha wrote:

> In D84519#2176909 <https://reviews.llvm.org/D84519#2176909>, @scott.linder wrote:
>
>> In D84519#2174866 <https://reviews.llvm.org/D84519#2174866>, @MaskRay wrote:
>>
>>> I am a bit worrisome that this patch is adding more target specific stuff to llvm-objdump.
>>
>> Agreed, I have looked at making the majority of llvm-objdump (and essentially everything under tools/) into a library at one point, but the biggest initial hurdle is the CommandLine parsing library. I think ideally all of the target-relevant parts would be in target-overridden implementations in some library, all of the llvm-objdump behavior parts (like the core symbol-to-symbol loop) would be in an llvm-objdump library which depends on the libraries with target-specific behaviors, and the tools/ directory would contain ~10 line main functions.
>>
>>> Can we step back and think whether precise CPU selection is really necessary? For example, PowerPC has the needs for different ISA levels but it simply uses -mcpu=future.
>>
>> I am a bit lost on what you mean; is there a significant cost to providing precise CPU selection? It doesn't make sense to require the user to e.g. run the dumper to inspect the binary in order to come up with the command-line to run the dumper in order to inspect the binary.
>>
>>> If different AMDGPU CPUs are incompatible, should such detection be moved to somewhere in lib/Target/AMDGPU/?
>>
>> Yes, I think it should be done in a generic way. All other targets which don't need/want it just getting the default (existing) behavior.
>
> **I'd like to point out that this patch correctly detects the CPU string by looking at the binary.** The CPU string needs to be determined beforehand and passed on to the SubtargetInfo constructor.
>
> To move the CPU string detection to lib/Target/AMDGPU, the target would also need to look at the binary. I think, one would have to pass `ObjectFile *` to the `MCSubtargetInfo` constructor. This allows targets to do the needful in their respective constructors. I'd like to know thoughts/opinions on this.

Maybe `lib/Target/*` is not the right place for the code then? If the dependency is on `Object` then where would the logical place for the code to live be?

I think "anywhere but llvm-objdump.cpp" is the concern, but I don't know where the right library is for target-specific logic related to Objects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519