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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
