Rebased prior to commit

In D92105#3474670, @reames wrote:

Drive by review. I'm mostly using this as an exercise to get familiar with the code and the target, so please don't consider any of my comments blocking.

Rebased including an update from using isShiftedUIntN to the templated replacement isShiftedUInt<N, S>. Addressed the issue of inserting incorrect instructions to copy FP registers to compressible FP registers by using FSGNJ_S and FSGNJ_D instead. We note that we can't have an additional offset to adjust the resulting register by if it is an FP register.

In D76007#3412653, @arsenm wrote:

In D76007#3310559, @lewis-revill wrote:

In D76007#3308144, @arsenm wrote:

Register banks do not have sizes. A register bank mapping is a bank plus a size/offset that you can change based on the target's size, not part of the bank itself.

Right. But before this patch there was a max size associated to the bank itself, which is queried by the verifier. And if we want to keep that concept of a maximum size surely we should have it be accurate? So for different hardware modes it should be possible for it to be different.

Why is there a maximum size concept? What is it used for? I haven't noticed thisb efore

In D76007#3412660, @arsenm wrote:

In D76007#3412653, @arsenm wrote:

In D76007#3310559, @lewis-revill wrote:

In D76007#3308144, @arsenm wrote:

Register banks do not have sizes. A register bank mapping is a bank plus a size/offset that you can change based on the target's size, not part of the bank itself.

Right. But before this patch there was a max size associated to the bank itself, which is queried by the verifier. And if we want to keep that concept of a maximum size surely we should have it be accurate? So for different hardware modes it should be possible for it to be different.

Why is there a maximum size concept? What is it used for? I haven't noticed thisb efore

If it is a maximum, why can't you set to the maximum possible size?

In D118766#3308106, @arsenm wrote:

Can you add a verifier test for this?

The bridging between LLT and MVT is increasingly painful, we should probably start putting in the effort to move more infrastructure to directly using LLT

In D76007#3308144, @arsenm wrote:

Register banks do not have sizes. A register bank mapping is a bank plus a size/offset that you can change based on the target's size, not part of the bank itself.

Bump - is anyone able to review this?

Bump - is anyone able to review this?

Abandoned in favour of D118766

In D76007#3249742, @arsenm wrote:

I'm not sure I understand why the RegisterBankInfo needs to be aware of the hwmode. HWmode changes the size of the classes during selection, but the registerbankinfo just needs to assign sizes to banks. The concrete classes don't matter so much

Restructure slightly, add more cases for selectCopy along with tests, add selection patterns for RV64 W instructions.

Remove unnecessary loop checking legality

Address review comments; primarily avoid selecting directly until the instruction selector.

Use ValueAssigners to avoid rewriting generic infrastructure and use determineAndHandleAssignments instead.

Introduce an IncomingValueAssigner to allow us to call determineAndHandleAssignments.

Don't reinvent existing generic functions; instead introduce an OutgoingValueAssigner to allow us to call determineAndHandleAssignments.

Still need to address testing permutations of copying virtual/physical registers.

Rebased. Added the first GIComplexPatternEquiv definition in order to handle the operands to shifts, which are now covered by a ComplexPattern in SelectionDAG. Updated the tests to match the output of the regbank selector.

Rebased. Updated tests to match the output of the legalizer.

Rebased.

Rebased.

Rebased, and updated where appropriate to continue to build correctly.

Rebased, and updated where appropriate to continue to build correctly.

In D91931#2410405, @StephenFan wrote:

In D91931#2410204, @craig.topper wrote:

There's already a series of patches to add more GlobalISel support for RISCV. For example, https://reviews.llvm.org/D76445. The others can be found in the stack of patches attached to that one.

I noticed that these patches have been silent for several months, how can I promote this?

Sure I'll land these apologies for the delay..

See nitpick, otherwise LGTM

LGTM

See nitpicks, otherwise LGTM

LGTM

Thanks Paolo, tests are all passing and apart from the nitpicks this is a green light from me, as with the rest in this series.

Bumping this patch - Looking at the output of the riscv-expand-pseudos pass I'd say these values are accurate. I'd say comments here, and in the functions which expand the instructions to ensure things don't get changed - seems to me like tests would be a little tricky so perhaps omitting that would be fine just to get this landed.

Bug fix - ensure selectConstant produces copies with fully constrained registers.

Bug fix - don't try to access the size of a $noreg register.

Add target flags to calls in tests.

Bug fix - add target flags to call instruction.

Add custom selection for copies and for constants. Significantly expand tests over more types.

Add more operand mappings and expand tests over more types.

Tweak test order

Add testing for shifts. Include a version of handling single word operations on RV64 involving directly lowering to the final opcode.

Expand patch to include operations for other types as introduced by the legalizer.

Expand patch to correctly cover more methods of types being legalized - IE: single word instructions on RV64, split operations (add with carry etc.) and libcalls up to s128.

Use utils/update_mir_test_checks.py

Use utils/update_mir_test_checks.py

Use utils/update_mir_test_checks.py

Removed unnecessary line

Add tests for AND/OR/XOR.

Support AND/OR/XOR.

Support AND/OR/XOR.

Rebased.

Address comments relating to supported types.

Rewrote the global RegBanks array to be constant. Instead of modifying RegisterBanks after construction, provide HwMode to the constructor of RegisterBankInfo and move size calculation logic for RegisterBanks to that class.

I'm lacking an understanding of how concurrency is expected to work within LLVM, but since there is only one RegisterBankInfo constructed per subtarget I don't see how there is a problem with updating the global RegBanks array? Likewise the HwMode is constant for the subtarget, so the write will be of the same value.