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