This LGTM, I've left a note on a potential comment update, but otherwise looks good

Is there value in still having most of these tests without the pattern? Some of these now look like tests just of andn, and in others now that enabling b or zbs has no effect on code generation. I wonder if this change should instead just completely remove the edited tests. If future additions make them relevant again they could later be readded.

Update for bitmanip 0.93

• Update to bitmanip 0.93
• Expand and support vector as per workaround in D95146
• Add negative testing (check __riscv_b not defined for just subextension)

• Emit extensions in canonical order
• Update V tests to match current compiler support
• Add tests for canonical order/parsing multiletter with version numbers
• Fix parsing issue found by updated tests

In D95146#2514390, @kito-cheng wrote:

So how about GCC/binutils, GNU toolchain using a weird work-around here, v implied zvamo and zvlsseg, and zvamo/zvlsseg/zvqmacc implied v...

In D95146#2512871, @craig.topper wrote:

Doesn't this mean that if you only enable zvlsseg, you'll be able to use the instruction in that extension but not the vsetvli instruction that you need to program the VL register?

Have 'b'/'v' features imply subfeatures

In D94931#2506141, @kito-cheng wrote:

I think maybe we could extract the arch parser from driver[1] to llvm/lib/Support, so that we could just maintain one parser for driver and assembler, and there is also other potential user for that, like the C front-end for the target attribute, e.g. __attribute__ ((target ("arch=rv64gcv"))), and the linker can re-use that to read/merge/write the attribute too.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L171

Add support to llvm/lib/Object/ELFObjectFile.cpp

In D94931#2506053, @HsiangKai wrote:

Do you consider to modify ELFObjectFileBase::getRISCVFeatures() in llvm/lib/Object/ELFObjectFile.cpp?

Rebase on D94930 to show updated version

In D94403#2504046, @kito-cheng wrote:

Thanks you implement that on clang, I think it's really great to included that in LLVM 12 release.

I would like to define marco for sub-extension too, I know that's my fault, I didn't specify the behavior of sub-extension clearly on the spec, but I think it would be great if we also define sub-extension marcos, since it would be easier to check when some core only implement sub-extension, and the code can just check the sub-extensio rather than check both.

In D94568#2495052, @kito-cheng wrote:

RVB 0.93 is an awkward version to me, there is mnemonic conflict which is not resolved during release process since it's kind of too rush, the conflict one is bext in zbe and zbs...

However it's also a milestone for B-ext, since this version claim zba, zbb and zbc is frozen, maybe those 3 sub-ext. could be removed from the umbrella of -menable-experimental-extension ?

@asb What do you think about this?

Correct constant used in macros, 2.0 should be 2000000 not 20000000

In D91717#2411334, @edward-jones wrote:

In D91717#2407884, @luismarques wrote:

It seems a bit excessive to me to coalesce the entry points into bundles of 4. Do you have any particular benchmarking data or reasoning that supports choosing that threshold?
Also, shouldn't this implementation include CFI directives?

I used bundles of 4 just to follow the behaviour I saw in libgcc, and the grouping of 2 for rv64 seemed a bit too fine-grained. I'm not sure what the original justification for the coalescing into groups of 2/4 was in libgcc.

I'll update to account for other suggested changes and see if I can find any benchmarks which show the tradeoff for the grouping threshold

Thanks for reviewing. I'll rebase and add this shortly.

Rebase

Thanks for looking at this @luismarques We had planned to put more effort into this patch, but time got in the way for quite a lot, but I'm glad it's working; thanks for the rebase I'll update this to match shortly. And it's good that it looks like it's mostly working. I'm curious about your backtrace showing one frame, is that something without debug information, since the example I was using when writing this did show a backtrace back to main? It would be good to understand why that disn't produce the expected output.

In D80802#2156221, @HsiangKai wrote:

The modification is put in D81213.

Since this patch replaces 0.8 support with 0.9, it should include an update to the version check in clang/lib/Driver/ToolChains/Arch/RISCV.cpp to match.

It's a shame this just missed the creation of the llvm 11.0 branch, do we think it's worth trying to get this backported since it only just missed?

I'm not familiar with the vector extension, but given the title of the patch, I have an integration question: It looks like this is enabled by use of the vector target feature, but the name 'Zvlsseg' suggests it's something optional/extra. If the intent to have this enabled unconditionally with 'v', or does it make sense to add features like what was done for bitmanip, where each 'Zb*' part can be enabled/disabled indepenently?

Thanks for this Ed. I've tried building a toolchain with it and noticed a couple of things:

In D80367#2049179, @MaskRay wrote:

Why does RISC-V need PrintRequiresAddr?

In D78702#2011633, @apazos wrote:

Simon can you please rebase, it seems D78776 got merged and now conflicts. Thank you.

This is looking good, overall the patch is nicely laid out which has made it easy to compare against the spec.

Looking at the structure this is looking good, it's probably worth renaming the codegen tests to match what I did with the MC patch before commiting ('Z'->'z') but other than a few formatting changes, this seems good. I haven't yet read through all the patterns, I'll add more comments as I go through it.

In D76767#1970892, @apazos wrote:

To clarify, we were not missing in compressed instructions before, this change is for the future compressed instructions to be added, confirmed? Otherwise I need to check why we missed it with the fuzzer.

In D77567#1970868, @apazos wrote:

Thanks, Simon for pushing this patch, it does help when debugging code and removes the dependence on binutils.

In the future patch for other instructionsm are you including lui/addi combo and addi with gp?

Resolve issue found by clang-tidy

Rebase on updated MC patch

In D65649#1943742, @asb wrote:

I'm reviewing this with an eye to merging it, but one big thing that comes to mind is the compressed instructions. The draft bitmanip spec describes these under "Future compressed instructions" and says "It presumably would make sense for a future revision of the “C” extension to include compressed opcodes for those instructions." My reading is that this is more of a sketch of potential encodings and a less firm proposal than the 32-bit encodings described elsewhere in the spec. Do you disagree with that assessment?

Actually solve the underlying problem.

Context for this patch: it's needed for CompressPats for bitmanip - since one of the compressed instructions should match against -1

This is starting to look good. I've checked all the encodings, and other than c.zext.w having the wrong value all the encodings are right.

Address own feedback on RISCV.td and cpp files

Address feedback

Address review comments and clang-format changes

Rebase on new dependencies

• Update to match latest dependencies
• Handle adding "experimental-" to SubtargetFeatures for experimental features

Rebase on updated D65649

• Update patch now AssemblerPredicate update has landed
• Add "experimental-" to all bitmanip SubtargetFeatures

Patch makes sense, and looking at the newly generated XRegisterBank.inc files, all LGTM

In D74338#1915415, @evandro wrote:

It'd be nice if a single predicate wouldn't need all_of, but I don't feel strongly about it.

Following feedback from @nhaehnle (http://lists.llvm.org/pipermail/llvm-dev/2020-February/139186.html) I have re-designed this to look more TableGen like (i.e. using DAGs instead of strings to describe the predicate). I have declared two operators any_of and all_of which I think read better than and and or in these cases when just looking at the target files.

In D74338#1875026, @evandro wrote:

It's a logical addition to the semantics of features. Though at the moment RV is probably the only user of this addition, the eventual patch implementing it should not include the RV specific changes, which would be better in a separate patch.