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

[Zakk Chen via Phabricator via cfe-commits]

khchen added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:835
+    llvm::RISCVISAInfo::filterISAStrings(Features);
+    std::vector<llvm::Metadata *> Ops;
+    if (Features.empty()) {
----------------
jrtc27 wrote:
> 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.
Because I think maybe the merge behavior `AppendUnique` is the best way to merge two module with different value of metadata flag, and it requires a metadata node with operands list.
Personally, I did not like to to have a new merge behavior to merge two target-features string, but maybe there is another better merging behavior I never thought,

In addition, some target features are not related to extension information, for example, +relax, +save-restore. Should we really need to record those unnecessary information in module?

What do you think?






================
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;
----------------
jrtc27 wrote:
> 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.
What's excepted elf attribute if llc with -mattr=+f,+d but the riscv-isa-features module flag is +a,+c+m?
I think it will be +f, +d, so we need to make sure -mattr is empty before use the module flag.  Does it make sense?

> The module flag should have been parsed and applied to the subtarget long ago. 
Sorry, I didn't understand it. The module flag `riscv-isa-features` is not applied to subtarget, I only use it to generate the correct extension .attribute. 

Maybe I misunderstood something?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106347