[PATCH] D106347: [PoC][RISCV] Encode arch information in a new module flag meatadata 'riscv-isa-features'.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.


================
Comment at: clang/test/CodeGen/RISCV/riscv-metadata-isa-features-empty-target-feature.cpp:9
+// We need to record extension target-feature in riscv-isa-features module flag metadata because there is some empty target-features attribute
+// CHECK: !{{[0-9]+}} = !{i32 6, !"riscv-isa-features", !{{[0-9]+}}}
+// CHECK: !{{[0-9]+}} = !{!"+d", !"+f"}
----------------
jrtc27 wrote:
> If you're not going to change the representation to be a single string, at least use a FileCheck variable so you can assert that this list is the one referenced by the module attribute above.
No check that the module flags actually includes this?


================
Comment at: clang/test/CodeGen/RISCV/riscv-metadata-isa-features-empty-target-feature.cpp:9-10
+// We need to record extension target-feature in riscv-isa-features module flag metadata because there is some empty target-features attribute
+// CHECK: !{{[0-9]+}} = !{i32 6, !"riscv-isa-features", !{{[0-9]+}}}
+// CHECK: !{{[0-9]+}} = !{!"+d", !"+f"}
+
----------------
If you're not going to change the representation to be a single string, at least use a FileCheck variable so you can assert that this list is the one referenced by the module attribute above.


================
Comment at: clang/test/CodeGen/RISCV/riscv-metadata-isa-features.c:15-21
+// BASE-ISA: !{{[0-9]+}} = !{!""}
+// RV32D-ISA: !{{[0-9]+}} = !{!"+d"}
+// RV32FD-ISA: !{{[0-9]+}} = !{!"+d", !"+f"}
+// RV32FDVZFH-ISA: !{{[0-9]+}} = !{!"+d", !"+experimental-v", !"+experimental-zfh", !"+f"}
+// RV64D-ISA: !{{[0-9]+}} = !{!"+d"}
+// RV64FD-ISA: !{{[0-9]+}} = !{!"+d", !"+f"}
+// RV32-NEG-D: !{{[0-9]+}} = !{!"-d"}
----------------
These need check lines to ensure that these are actually being used for the right attribute. Especially `!{!""}`, that could easily be something else that you happen to match.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:184
 }
-
 void RISCVAsmPrinter::emitEndOfAsmFile(Module &M) {
----------------
Don't delete this


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:187
+        dyn_cast_or_null<MDNode>(M.getModuleFlag("riscv-isa-features"));
+    if (STI->getFeatureString().empty() && ISAFeatureNodes) {
+      std::vector<std::string> Features;
----------------
This doesn't make sense. The module flag should have been parsed and applied to the subtarget long ago. And an empty feature string means no features, not that they're missing. The fact that you need to change this code here is a sign that code elsewhere is wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106347