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

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha marked 3 inline comments as done.
rochauha 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 { }
----------------
rochauha wrote:
> scott.linder wrote:
> > 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`.
> Since `getArch` / `getFeatures` were not using `Optional`, I thought maybe the convention should be maintained. But I do agree that `Optional` is the more //self documenting// interface in a way.
Changed to using  `Optional`.


================
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.
----------------
scott.linder wrote:
> 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.`
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519