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

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha 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
----------------
scott.linder wrote:
> 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.
I don't have any strong opinion regarding this. The kernel is empty anyways.  I chose `-O0` with the hope that the binary size would be slightly bigger than otherwise.


================
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");
----------------
scott.linder wrote:
> 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.
>From what I understand, TargetStreamer is used to emit binaries. But here we are going the other way round, we need to infer the details from the binary header. 

Is it possible to move `getArchNameFromElfMach()` to lib/Support/TargetParser.*? It would certainly make this patch cleaner. And the code would also be used across AMDGPUTargetStreamer and llvm-objdump.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519