[PATCH] D94931: [RISCV] Add attribute support for all supported extensions

[Kito Cheng via Phabricator via llvm-commits]

kito-cheng added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2155
+      if (getFeatureBits(RISCV::FeatureStdExtZvlsseg))
+        formalArchStr = (Twine(formalArchStr) + "_zvlsseg0p9").str();
 
----------------
Those attribute seems not output in canonical order, IIRC ld.bfd will check that during link stage.

e.g. when we have `zfh` and `zbb`, it should be `rv32i_zfh_zbb` rather than `rv32i_zbb_zbf` since the `f` > `b` in canonical order.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:92
+  if (STI.hasFeature(RISCV::FeatureStdExtZvlsseg))
+    Arch += "_zvlsseg0p9";
 
----------------
Same as above here.


================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:49
 ; RV32C: .attribute 5, "rv32i2p0_c2p0"
+; RV32B: .attribute 5, "rv32i2p0_b0p92_zbb0p92_zbc0p92_zbe0p92_zbf0p92_zbm0p92_zbp0p92_zbr0p92_zbs0p92_zbt0p92"
+; RV32V: .attribute 5, "rv32i2p0_v0p9"
----------------
jrtc27 wrote:
> asb wrote:
> > I suppose given the way this works in LLVM it would add even more duplication to reverse the logic for enabling sub-extensions and specify only `rv32i2p0_b0p92`, but @kito-cheng, can you please confirm what GCC does in a case like this?
> A single (extension -> extension list) list for all the implications would work for both uses I think?
GCC & binutils will always expand the extension if it consisted by several sub-extensions, so yes, it doing same as GCC here.


================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:50
+; RV32B: .attribute 5, "rv32i2p0_b0p92_zbb0p92_zbc0p92_zbe0p92_zbf0p92_zbm0p92_zbp0p92_zbr0p92_zbs0p92_zbt0p92"
+; RV32V: .attribute 5, "rv32i2p0_v0p9"
+; RV32ZBB: .attribute 5, "rv32i2p0_zbb0p92"
----------------
I expect it should be v0p9_zvamo0p9_zvlsseg0p9 since V implied `zvamo` and `zvlsseg`.


================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:63
+; RV32ZVAMO: .attribute 5, "rv32i2p0_v0p9_zvamo0p9"
+; RV32ZVLSSEG: .attribute 5, "rv32i2p0_v0p9_zvlsseg0p9"
 ; RV64M: .attribute 5, "rv64i2p0_m2p0"
----------------
`rv32i2p0_zvlsseg0p9` rather than `rv32i2p0_zvlsseg0p9` here, it's kind of weird, but in theory `zvlsseg` is not implied `v`.

`zvamo` is same situation too.


================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:83
+; RV64ZVAMO: .attribute 5, "rv64i2p0_v0p9_zvamo0p9"
+; RV64ZVLSSEG: .attribute 5, "rv64i2p0_v0p9_zvlsseg0p9"
 
----------------
Ditto.


================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:84
+; RV64ZVLSSEG: .attribute 5, "rv64i2p0_v0p9_zvlsseg0p9"
 
 define i32 @addi(i32 %a) {
----------------
I would suggest you add some combination of zb*, zv* and zfh to make sure it output in canonical order.


================
Comment at: llvm/test/MC/RISCV/attribute-arch.s:79
+.attribute arch, "rv32izvamo"
+# CHECK: attribute      5, "rv32i2p0_v0p9_zvamo0p9"
+
----------------
No `v0p9` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94931