[PATCH] D84519: [llvm-objdump][AMDGPU] Detect subtarget

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/subtarget.ll:9
+;
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1030 -filetype=obj -O0 -o %t.o %s
+; RUN: llvm-objdump -D --arch-name=amdgcn --mcpu=gfx1030 %t.o > specify.txt
----------------
Would it make sense to have tests at various optimization levels? Generally we seem to prefer writing tests at the default opt level unless the test is explicitly checking something at a certain opt level.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2075
+  const ELF64LEFile *EF = cast<ELFObjectFile<ELF64LE>>(Obj)->getELFFile();
+  auto *TheElfHeader = EF->getHeader();
+  assert(TheElfHeader->e_machine == ELF::EM_AMDGPU);
----------------
Between the lint and not knowing off-hand what `getHeader` returns, I'd probably just not use auto here.

I think the full type can be as simple as `ELF64LE::Ehdr`?

I would also prefer dropping `The`, unless it is there to avoid colliding with another name?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2079-2113
+  // Compare codes in descending order.
+  if (IS_AMDGPU_CPU(EF_AMDGPU_MACH_AMDGCN_GFX1030))
+    return StringRef("gfx1030");
+  if (IS_AMDGPU_CPU(EF_AMDGPU_MACH_AMDGCN_GFX1012))
+    return StringRef("gfx1012");
+  if (IS_AMDGPU_CPU(EF_AMDGPU_MACH_AMDGCN_GFX1011))
+    return StringRef("gfx1011");
----------------
Can this just be `StringRef ArchName = AMDGPUTargetStreamer::getArchNameFromElfMach(e_flags && EF_AMDGPU_MACH); return ArchName.empty() ? None : ArchName`?

If there is an issue with making that function visible here I would propose just moving the implementation, it seems like a pretty general thing.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2148
+
+  auto TargetCPUString = getTargetCPUIfPossible(Obj);
+  if (MCPU.empty() && TargetCPUString)
----------------
I would sink this under the condition for `MCPU.empty()` if you only need it in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519