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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:330
   virtual SubtargetFeatures getFeatures() const = 0;
+  virtual StringRef getTargetCPUName() const { return StringRef(""); };
   virtual void setARMSubArch(Triple &TheTriple) const { }
----------------
Can you document this, noting that it returns an empty `StringRef` when no CPU can be detected? I am also curious why you dropped the `Optional`, it seemed like a nicer interface that made the documentation of the "empty" case part of the code. I suppose in C++ there is no way to elide the extra storage for the `Optional`, but I would still think it is worth using.

I would also prefer to drop `Target` from the name, as `getArch`/`getFeatures` do not mention `{Sub}Target`.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:369
+  using namespace ELF;
+  unsigned EF_CPU = getPlatformFlags() & EF_AMDGPU_MACH;
+  switch (EF_CPU) {
----------------
I would prefer to just name this following the style guide, I don't think this is a case where breaking it helps readability.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2093
+
+  // If MCPU isn't specified, detect it. Right now only AMDGPU implements the
+  // detection. In other cases, an empty string is returned.
----------------
I don't think we need to even mention "AMDGPU" here, and the bit about an empty string being returned in the "failure" case should just be in the doc-comment on `getTargetCPUName`; I would instead just indicate the call is fallible by adding "try" somewhere.

All in all, I'd just replace this whole comment with `// If MCPU Isn't specified, try to detect it.`


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2095
+  // detection. In other cases, an empty string is returned.
+  MCPU = MCPU.empty() ? Obj->getTargetCPUName().str() : MCPU;
+
----------------
I think using the ternary here actually makes this longer/harder to read compared to an `if` (for example, you need the explicit call to `.str()` rather than getting the `std::string(StringRef)` constructor implicitly).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519