[PATCH] D139553: [llvm-objdump][RISCV] Use new common method to parse ARCH RISCV attribute

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Looks great with some nits.



================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:305
       Attributes.getAttributeString(RISCVAttrs::ARCH);
+
   if (Attr) {
----------------
The common style doesn't add a blank line after a variable declaration.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/RISCV/objdump-instr-from-extensions.s:1
+# Objdump should decode instructions that are supported by extensions that are used in RISCV_ARCH attribute.
+
----------------
For binary utilities, new `test/tools/llvm-*` tests are supposed to use `## ` for non-RUN-non-CHECK lines.

Nitpicking: `objdump-` in `objdump-instr-from-extensions.s` doesn't convey additional value as the test directory has `llvm-objdump` in the component.
I don't have a particular good name, and hope others can make a suggestion. Perhaps `Tag_RISCV_arch.s` is sufficiently clear.

llvm-objdump isn't objdump (which usually refers to GNU objdump).

Use the canonical name `Tag_RISCV_arch`.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/RISCV/objdump-instr-from-extensions.s:24
+
+# CHECK: vfsub.vf v8, v4, fa0, v0.t
+vfsub.vf v8, v4, fa0, v0.t
----------------
IIUC vfwsub/vfrsub are in the same family and it's too useful to have more than one instructions in one family for the purpose of this test. Perhaps just keep `vfsub.vv` and delete all lines below (and the blank line above).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139553