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

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha added inline comments.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:382
+
+    // Radeon HD 4000 Series (R700).
+  case ELF::EF_AMDGPU_MACH_R600_RV710:
----------------
MaskRay wrote:
> Why is the comment not aligned with `case `?
This is the result after running clang-format.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/subtarget.ll:1
+define amdgpu_kernel void @test_kernel() {
+  ret void
----------------
MaskRay wrote:
> In these binary utility directories, we usually place RUN/CHECK lines before the text.
Since the function is pretty small, I thought it would be helpful to have it first.
Do you think the layout should be changed?


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/subtarget.ll:5
+
+; Test subtarget detection. Disassembly is only supported for GFX8 and beyond.
+;
----------------
MaskRay wrote:
> In these binary utility directories, we use `;; ` for non-RUN-non-CHECK comments. The different comment marker makes comments stand out.
I looked at a few tests in the binary utility tests but didn't find this pattern. Do you suggest that this should be changed in this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84519