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.

My initial thoughts are that this may only be an issue if there can be potentially more than one instance of RegisterBankInfo? And indeed, if it is possible for each of these to have been constructed with differing HwModes? I'll try and look into it in more detail and find out.

Add XLenLLT, take into account M extension.

Address comments

Add test for pointer return type

Fix copy/paste mistakes in check lines

All looks good apart from some test nitpicks. Also add more reviewers

This looks good to me, as long as it covers the spec. Can you add more reviewers though?

Thanks, this is looking in good shape now. As long as everyone agrees on this scheme I think the implementation is good to go (pending the bitmanip extension support).

Clang format

Rebase, update test structure.

Rebase, update test structure. Correct missing defs by adding CallReturnHandler.

I can't see any issues with the logic of this implementation. It doesn't lend itself well to adapting it in the future if we want to be able to combine the operators, but I'd expect that would require a more disruptive rewrite so I'm not too concerned about that right now. See comments though.

Since the DebugInfo fix has been accepted, I'm looking to get this patch and that fix committed shortly if there are no problems caused by rebasing.

Updated comment and added brief testcase, which now depends on D62686

Rebased

Thanks all for the feedback, I'll make this patch depend upon the save/restore feature so that I can add a testcase.

In D71774#1821760, @lenary wrote:

This seems like a fun issue:

• addi is compressible
• xor is almost certainly easier to analyse (from the view of KnownBits and the like).

Have you seen any regressions in code generation from this change?

Thanks for addressing the comments, I'd like some input from others before saying this looks good.

Well spotted, this seems like a good change to me. I wonder if there are other optimization opportunities to use this NegImm/simm12_plus1 pattern in further patches?

Thanks, this seems like a good change. I haven't checked the source, but I wonder is it ever possible this hook triggers duplication of the returns but the tail calls don't get emitted for whatever reason? Other than that and the comment above I'd be happy with this.

In D62686#1820816, @apazos wrote:

Lewis, your latest patch looks good, we just had another run with no new failures. But we know it will have issues with -g. So I think we should not merge it yet. Do you have a version of the patch that creates the labels for the compiler-generated save/restore lib calls, so that this optimization does not depend on D71593? We could merge that version then, and when D71593 is accepted, you just have to rework/remove the label generation part of the patch.