[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/lib/CodeGen/CodeGenModule.cpp:835
+    llvm::RISCVISAInfo::filterISAStrings(Features);
+    std::vector<llvm::Metadata *> Ops;
+    if (Features.empty()) {
----------------
Why is this building a list? Just use a string so it's in the same format as the function attributes? We already have parsers for that. Yes, it means you do a small amount of redundant work but having a single format in the IR for representing the same thing in every place is better than having multiple different ways of representing the same thing.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:843
+    }
+    getModule().addModuleFlag(llvm::Module::AppendUnique, "riscv-isa-features",
+                              llvm::MDNode::get(Ctx, Ops));
----------------
Just like we call it target-abi not riscv-abi, this should just be called target-features.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:132-142
+void RISCVISAInfo::filterISAStrings(std::vector<std::string> &Features) {
+  erase_if(Features, [](std::string &Feature) {
+    StringRef ExtName(Feature);
+    assert(ExtName.size() > 1 && (ExtName[0] == '+' || ExtName[0] == '-'));
+    ExtName = ExtName.drop_front(1); // Drop '+'
+    stripExperimentalPrefix(ExtName);
+    if (filterSupportedExtensionInfosByName(ExtName).empty())
----------------
Why do we need to do any filtering? Just emit the whole thing.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:53-60
+  emitTargetAttributes(ISAInfo);
+}
 
+void RISCVTargetStreamer::emitTargetAttributes(const RISCVISAInfo &ISAInfo) {
+  if (ISAInfo.hasExtension("e"))
+    emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_4);
+  else
----------------
This is unrelated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106347