This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Refactor lower to S[LR]I optimization
ClosedPublic

Authored by PetreTudor on May 1 2020, 4:48 AM.

Details

Summary

The optimization has been refactored to fix certain bugs and
limitations. The condition for lowering to S[LR]I has been changed
to reflect the manual pseudocode description of SLI and SRI operation.
The optimization can now handle more cases of operand type and order.

Diff Detail

Event Timeline

PetreTudor created this revision.May 1 2020, 4:48 AM

The problem with the original patch was that ElemMask would overflow, since it was only 32-bit wide. The patch now makes use of APInt to avoid this type of situation.

efriedma added inline comments.May 1 2020, 11:04 AM
llvm/test/CodeGen/AArch64/arm64-sli-sri-opt.ll
1–2

Please generate the test using update-llc-test-checks.py

Thanks for the updates. Looks like it's picked up some new tricks since the last version.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
8017

Can you add a comment saying this reconstructs the immediate from the BIC

8020

I think if this used C1 = ~(C1nodeImm->getZExtValue() << C1nodeShift->getZExtValue()) here, then the IsAnd condition below can be removed.

PetreTudor updated this revision to Diff 261808.May 4 2020, 7:18 AM

Addressed reviewers' comments.

Added comments, simplified some logic and updated llc tests via
update_llc_test_checks.py

PetreTudor marked 3 inline comments as done.May 4 2020, 7:19 AM

Thanks. This looks good to me, but I remember Eli making some comments on the other patch. Please wait for him to comment again too.

efriedma accepted this revision.May 5 2020, 10:37 AM

LGTM

It's not obvious to me from reading the testcase that we have test coverage code for both the BICi and the isAllConstantBuildVector case; please check before merging.

Probably it would be worth looking into pattern-matching cases that don't involve precisely the AND you're looking for, using computeKnownBits. But that doesn't need to happen here.

This revision is now accepted and ready to land.May 5 2020, 10:37 AM

Added a test for the isAllConstantBuildVector case. The BICi case happens
when the constant values' sizes are greater than 8 bits.

This revision was automatically updated to reflect the committed changes.