This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Switch to the Machine Scheduler
ClosedPublic

Authored by luismarques on Aug 29 2019, 1:31 PM.

Details

Summary

Most of the test changes are trivial instruction reorderings and differing register allocations, without any obvious performance impact.

One concern is that some reorderings break relaxable instruction pairs, such as lui/addi with %*hi and %*lo relocations. (Grepping with grep -C1 "%.*hi" *.ll in llvm/test/CodeGen/RISCV shows that in most tests such instructions are still in sequential pairs). We might need a proper machine model to fix that, but that would be a separate patch.

A few tests that had to be manually updated are worth additional review attention:

rv64i-complex-float.ll
compress.ll
callee-saved-fpr32s.ll
callee-saved-fpr64s.ll
callee-saved-gprs.ll

Diff Detail

Event Timeline

luismarques created this revision.Aug 29 2019, 1:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 1:31 PM

Rebased on master.

%hi and %lo don't need to be adjacent to relax them; the ABI has been designed with this in mind (and bfd keeps to that) So long as the result of the %hi is always "consumed" by a relaxable %lo, things will work.

%hi and %lo don't need to be adjacent to relax them; the ABI has been designed with this in mind (and bfd keeps to that) So long as the result of the %hi is always "consumed" by a relaxable %lo, things will work.

Does this apply even if there is another relocation between %hi and %lo?

Three nits about changes to the testcases that go beyond just reordering.

I don't think this should block the patch from landing, but I do think that we should keep an eye on why they are happening, and hope to get rid of them when we introduce a schedule.

llvm/test/CodeGen/RISCV/atomic-cmpxchg.ll
2176

This patch does seem to have added some additional mv instructions in this test (and others). It's usually because there's some kind of two-way register copy happening, which now uses a temporary register.

llvm/test/CodeGen/RISCV/callee-saved-fpr32s.ll
109

I don't know where this came from, but having both flw ... %lo(var)(a0) followed by addi ... %lo(var) does not seem so amenable for linker relaxation, though @jrtc27 would have a better idea about this than me.

llvm/test/CodeGen/RISCV/callee-saved-gprs.ll
46

We seem to have gained a lw; sw pair here. Not entirely sure why. There are a few additions like this in this testcase.

jrtc27 added a comment.EditedSep 10 2019, 5:12 PM

%hi and %lo don't need to be adjacent to relax them; the ABI has been designed with this in mind (and bfd keeps to that) So long as the result of the %hi is always "consumed" by a relaxable %lo, things will work.

Does this apply even if there is another relocation between %hi and %lo?

Yes, it really doesn't matter. So long as the data dependency is there in the assembly (which it will be, because that's what we generate) it makes no difference *where* the instructions are. The relaxations available are: 1. deleting lui rd, %hi(x) and changing the addi rd, rs1, %lo(x) to addi rd, gp, %gprel(x) (if within 2K of gp) or addi rd, x0, %lo(x) (if in first 2K of memory) 2. changing lui rd, %hi(x) to c.lui rd, %hi(x) (but a R_RISCV_RVC_LUI) if rd is a valid register and %hi(x) is a valid immediate for c.lui. None of these care about adjacency or order; they could even be split across object files.

Footnote: I only talk about addi here; any I-type or S-type instruction works in its place.

jrtc27 added inline comments.Sep 10 2019, 5:36 PM
llvm/test/CodeGen/RISCV/callee-saved-gprs.ll
46

I assume these were previously scheduled after the addi but have now been hoisted. Note that these check lines do not continue to the end of the function, these are truncated, and manually created rather than using update_llc_test_checks.py for this file.

lenary accepted this revision.Sep 12 2019, 2:53 AM

Ok, I'm happy with the test changes in this patch. Let's get it landed!

This revision is now accepted and ready to land.Sep 12 2019, 2:53 AM

Refresh patch before committing.

MaskRay closed this revision.Sep 17 2019, 2:57 AM

@luismarques Closed by rL372092 (unfortunately that commit does not include the summary here).

llvm/test/CodeGen/RISCV/atomic-cmpxchg.ll