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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Looking good to me! Some small nits, and it would be good to hear back on what the general consensus on adding this to `Object` is.



================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:368
+StringRef ELFObjectFileBase::getAMDGPUCPUName() const {
+  unsigned CPU = getPlatformFlags() & ELF::EF_AMDGPU_MACH;
+  switch (CPU) {
----------------
Can you add `assert(getEMachine() == ELF::EM_AMDGPU)`; ?


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:372
+  case ELF::EF_AMDGPU_MACH_R600_R600:
+    return StringRef("r600");
+  case ELF::EF_AMDGPU_MACH_R600_R630:
----------------
I think there should be an implicit `StringRef(const char *)` constructor, so you can drop the explicit call here, same for the rest of the returns here.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/subtarget.ll:10
+; 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
+; RUN: llvm-objdump -D %t.o > detect.txt
----------------
shouldn't these `specify.txt` and `detect.txt` include `%t` somewhere in their path? I.e. `%t-specify.txt` and `%t-detect.txt`?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2095-2096
+  if (MCPU.empty()) {
+    auto CPUName = Obj->tryGetCPUName();
+    MCPU = CPUName ? CPUName.getValue().str() : MCPU;
+  }
----------------
Small nit, but I think this reads better as:

```
if (MCPU.empty())
  MCPU = Obj->tryGetCPUName().getValueOr("");
```

IMO the ternary operator just makes things more complicated, and you aren't exposing the intermediate type of `tryGetCPUName` anyway if you are using `auto`, so we don't lose anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519