[PATCH] D84519: [llvm-objdump][AMDGPU] Detect CPU string
Scott Linder via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 7 14:04:23 PDT 2020
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
More information about the llvm-commits
mailing list