This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Generalize logic for promoting copies
ClosedPublic

Authored by tambre on Apr 6 2020, 1:17 AM.

Details

Summary

Generalize the 16-bit FPR to 32-bit GPR logic to work for all cases where destination size is bigger than source size.

Also fixed CheckCopy() always returning true instead of the result of isValidCopy().

Diff Detail

Event Timeline

tambre created this revision.Apr 6 2020, 1:17 AM
tambre retitled this revision from [AArch64] Generalize logic for subregister copies to [AArch64][GlobalISel] Generalize logic for subregister copies.Apr 6 2020, 1:17 AM
tambre added a comment.Apr 6 2020, 1:49 AM

This actually breaks CodeGen/AArch64/f16-instructions.ll's test_select. Will investigate.
If you see anything incorrect in the current changes let me know. :)

paquette added inline comments.Apr 6 2020, 10:08 AM
llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
794–795

It's weird that we have this case and also the case above which checks getMinSizeForRegBank(SrcRegBank) > DstSize. Is this a typo?

paquette added inline comments.Apr 6 2020, 10:16 AM
llvm/test/CodeGen/AArch64/GlobalISel/opt-fold-ext-tbz-tbnz.mir
80 ↗(On Diff #255249)

I don't think this COPY is legal. This is a cross-bank copy from a 16-bit FPR onto a 32-bit GPR.

I think that we need a SUBREG_TO_REG here.

tambre updated this revision to Diff 255686.Apr 7 2020, 8:31 AM
tambre marked 4 inline comments as done.

Generalized SUBREG_TO_REG logic

Tests now pass.

tambre updated this revision to Diff 255687.Apr 7 2020, 8:33 AM

Fix formatting

tambre added a comment.Apr 7 2020, 8:34 AM

Added proper generalization for the DstSize > SrcSize aka SUBREG_TO_REG case. Also moved the missing comment to SrcSize > DstSize.
Tests now pass.

llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
794–795

The above is for cases where we can't perform a subregister copy from the source bank, e.g. 32-bit to 8-bit FPR.
This handles all other subregister copies.
And I've now added a third case for promoting copies (DstSize > SrcSize) as the generalization for the 16-bit FPR to 32-bit GPR case.

llvm/test/CodeGen/AArch64/GlobalISel/opt-fold-ext-tbz-tbnz.mir
80 ↗(On Diff #255249)

You're correct. Fixed.

tambre retitled this revision from [AArch64][GlobalISel] Generalize logic for subregister copies to [AArch64][GlobalISel] Generalize logic for promoting copies.Apr 7 2020, 8:46 AM
tambre edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B52160: Diff 255687.

paquette: ping

Where did the testcase go?

Where did the testcase go?

I didn't add any testcases. The testcase changes are now gone that I fixed the code to work correctly. This is now a NFC.

paquette: ping

This revision is now accepted and ready to land.Apr 23 2020, 10:23 AM

Please land this for me as I lack commit privileges.

This revision was automatically updated to reflect the committed changes.