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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D84519#2200683 <https://reviews.llvm.org/D84519#2200683>, @jdoerfert wrote:

> This seems to be related to the push of the `cpu-id` (aka. `target-id`) into other places as well: e.g., D84822 <https://reviews.llvm.org/D84822>, D80750 <https://reviews.llvm.org/D80750>, probably more.
> I mentioned in those reviews already, this doesn't strike me as an "AMDGPU" feature at all. 
> We should start the discussions around missing functionality instead of adding AMDGPU workarounds in all these places...
> One example is an IR module level `target cpu` and/or `target feature`, similar to `target triple`.
>
> ---
>
> Also, the commit doesn't have a message that explains what is going on.

I agree, but can we at least fix the ICE in llvm-objdump in the short-term in terms of the reality we are currently living in? I didn't look into this case closely before commenting last, but immediately above the diff in `disassembleObject` there is a call to `ObjectFile::getFeatures`, which is a pure virtual function implemented in the various concrete object file format implementations with reference to specific targets. For example, `ELFObjectFileBase::getFeatures` is implemented as:

  SubtargetFeatures ELFObjectFileBase::getFeatures() const {
    switch (getEMachine()) {
    case ELF::EM_MIPS:
      return getMIPSFeatures();
    case ELF::EM_ARM:
      return getARMFeatures();
    case ELF::EM_RISCV:
      return getRISCVFeatures();
    default:
      return SubtargetFeatures();
    }
  }

Which is just the same code Ronak is adding here, hoisted into the `Object` library. Again, this is not AMDGPU specific, but somewhere it has to be implemented in terms of AMDGPU (because we define the ABI of our code objects).

I worry that we will just have to live with broken tools for months while we try to fix up core parts of LLVM, and all the while we could make a small, well-defined change consistent with the existing logic in the `Object` library to get to something useful. The "precedent" for this sort of code in `Object` is then just https://reviews.llvm.org/D21125. If we come along soon after and implement a vastly superior, internally-consistent model of all of these things and update `Object`, we will have to have updated e.g. `getFeatures` anyway, so there is no loss.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519