This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Avoid copies to target register bank for subregister copies
ClosedPublic

Authored by tambre on Mar 1 2020, 3:58 AM.

Details

Summary

Previously for any copy from a register bigger than the destination:

  1. Copied to a same-sized register in the destination register bank.
  2. Subregister copy of that to the destination.

This fails for copies from 128-bit FPRs to GPRs because the GPR register bank can't accomodate 128-bit values.

Instead of special-casing such copies to perform the truncation beforehand in the source register bank, generalize this:
a) Perform a subregister copy straight from source register whenever possible. This results in shorter MIR and fixes the above problem.
b) Perform a full copy to target bank and then do a subregister copy only if source bank can't support target's size. E.g. GPR to 8-bit FPR copy.

Diff Detail

Event Timeline

tambre created this revision.Mar 1 2020, 3:58 AM
tambre retitled this revision from [AArch64][GlobalISel] Avoid unnecessary copies to target register bank for subregister copies to [AArch64][GlobalISel] Avoid copies to target register bank for subregister copies.Mar 1 2020, 3:58 AM
tambre edited the summary of this revision. (Show Details)
tambre added a comment.Mar 1 2020, 4:04 AM

This patch fixes bug 45029.

tambre edited the summary of this revision. (Show Details)Mar 1 2020, 4:06 AM
tambre updated this revision to Diff 247495.Mar 1 2020, 5:32 AM

Re-generate patch on Linux. Seems like line endings might've been messed up.

Jessica: Adding you as a reviewer, as you seem to have worked with this code.

I think this looks good; just a couple nits on the testcase.

llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
772

On calls to getMinClassForRegBank, can we add a comment like below?

getMinClassForRegBank(SrcRegBank, DstSize, /* GetAllRegSet = */ true)
llvm/test/CodeGen/AArch64/GlobalISel/subreg-copy.mir
4

This isn't necessary, because the runline already specifies the triple.

8–18

The actual IR here isn't necessary; it can probably just be

define void @test_128_fpr_truncation() { ret void }
paquette added inline comments.Mar 4 2020, 9:47 AM
llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
459

This should be an llvm_unreachable, I think. We never want to return anything here.

paquette added inline comments.Mar 4 2020, 9:53 AM
llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
658

While we're here, I think it would be good to add a couple asserts:

assert(SrcReg.isValid() && "Expected a valid source register?");
assert(To && "Destination register class cannot be null");
assert(SubReg && "Expected a valid subregister");
paquette added inline comments.Mar 4 2020, 10:16 AM
llvm/test/CodeGen/AArch64/GlobalISel/subreg-copy.mir
37

Do you have to use G_GLOBAL_VALUE here? Would this work instead?

bb.0:
liveins: $x0
  %1:gpr(p0) = COPY $x0

Then you could replace the G_LOAD with

%0:fpr(s128) = G_LOAD %1:gpr(p0) :: (load 16)

If you can do that, then you can delete the entire IR section in the testcase.

(You might have to add tracksRegLiveness: true to make this work)

43–49

Is anything after the COPY necessary to test the behaviour here? Can we just return the result of the COPY? E.g.

$x1 = COPY %8:gpr(s64)
RET_ReallyLR implicit $x1
tambre updated this revision to Diff 248270.Mar 4 2020, 11:52 AM
tambre marked 9 inline comments as done.

Address Jessica's review comments.

Thanks for the review Jessica! I addressed your comments, except for being unable to reduce the testcase much further.
Note that this change is a fix for bug 45029.

llvm/test/CodeGen/AArch64/GlobalISel/subreg-copy.mir
8–18

It seemed to me that the IR was usually included for reference. Seems not! :)

43–49

Changing anything after the COPY results in the testcase no longer crashing prior to this change. I tried reducing in various ways, but unfortunately no dice.

paquette accepted this revision.Mar 4 2020, 12:44 PM

LGTM!

This revision is now accepted and ready to land.Mar 4 2020, 12:44 PM
tambre added a comment.Mar 4 2020, 9:19 PM

Jessica, please land the patch for me. As this is my first contribution, I probably lack the permissions to do that myself.

This revision was automatically updated to reflect the committed changes.