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

[khchen]

Summary

riscv-isa-features records the march information for a module.
RISCVAsmPrinter reads the riscv-isa-features to generate corrent arch
attribute if -mattr is missing.

Currently RISC-V encodes arch information in function attribute
'target-features', so there are two issues we want to fix.

1. In IFUNC case (ex. https://godbolt.org/z/a1oTbacn9), the final arch

attribute information is ambiguous because one compilation unit have
different target-feature value in function attribute.
We could not just union of all target-features, because in IFUNC case,
the base arch string is excepted in elf arch attribute.

2. During the LTO, clang driver will not pass -march option to LTO code

generator, because users maybe specify the incorrect -march value if
some linked external libraries have been compiled with different arch
extensions.

The probably way is adding a new module flag metadata, riscv-isa-features,
to records -march info for one compilation unit, and each
riscv-isa-features get combined when linking.

In this PoC patch, I think the potential issue is illegal arch combination
will report an error in code generation stage, not in linking stage.
(For example, zfinx is conflict with f/d/q, and Zceb and Zcec are
conflict with d).
I'm not sure having a target dependence behaivor in bitcode linking does
make sense or not.

Thanks for @jrtc27, @MaskRay to address this issue in D102926 and
D102925, and @kito-cheng's reminder of exclusive extensions issue.

[jrtc27]

Why can't we just save target-features itself as a module flag instead of inventing yet another equivalent encoding? Especially since a long bitfield is brittle, you can't reorder or remove elements without breaking bitcode compatibility.

[khchen]

In D106347#2890123, @jrtc27 wrote:

Why can't we just save target-features itself as a module flag instead of inventing yet another equivalent encoding? Especially since a long bitfield is brittle, you can't reorder or remove elements without breaking bitcode compatibility.

I think the benefit of using bitfield is making link behavior simpler. I will try to investigate another link behavior to handle two different target-features strings especially have + or -.

[khchen]

store target-features string as module flag.

[jrtc27]

clang/lib/CodeGen/CodeGenModule.cpp

835

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.

843

Just like we call it target-abi not riscv-abi, this should just be called target-features.

llvm/lib/Support/RISCVISAInfo.cpp

132–142

Why do we need to do any filtering? Just emit the whole thing.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

53–60

This is unrelated

[jrtc27]

llvm/test/CodeGen/RISCV/riscv-isa-features.ll

1

Use update_llc_test_checks for these.

2

It's not overriding. It's just that #0 doesn't include -f,-d so applying #0 atop the default target-features keeps the default f and d features.

3

CHECK and ISA-F-D are not consistent with the check prefixes used in other RISC-V tests.

29

Space before }

[jrtc27]

clang/test/CodeGen/RISCV/riscv-metadata-isa-features-empty-target-feature.cpp

9

No check that the module flags actually includes this?

9–10

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.

clang/test/CodeGen/RISCV/riscv-metadata-isa-features.c

15–21

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.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

184

Don't delete this

187

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.

[khchen]

clang/lib/CodeGen/CodeGenModule.cpp

835

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?

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

187

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?