Page MenuHomePhabricator

[DAGCombine] Improve ReduceLoadWidth for SRL
ClosedPublic

Authored by samparker on Dec 18 2017, 5:45 AM.

Details

Summary

If the SRL node is only used by an AND, we may be able to set the ExtVT to the width of the mask, making the AND redundant. To support this, another check has been added in isLegalNarrowLoad which queries whether the load is valid.

I've had to change a couple of X86 codegen tests, but I really don't know if they're correct. @RKSimon, if you could check these that would be great.

Diff Detail

Repository
rL LLVM

Event Timeline

samparker created this revision.Dec 18 2017, 5:45 AM

All of the current tests look good to me. I think you should add some tests for non-byte shifts.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3793 ↗(On Diff #127344)

What if ShAmt isn't a multiple of 8?

Good point, I will add some tests for that.

samparker updated this revision to Diff 127380.Dec 18 2017, 9:23 AM

Added some tests to handle non-byte shifts and masks, which highlighted that those cases weren't handled! This fix means that I've reverted one of the x86 test changes too.

RKSimon added inline comments.Dec 18 2017, 10:33 AM
test/CodeGen/X86/h-registers-1.ll
18 ↗(On Diff #127380)

These checks aren't testing the zero-extension. I've updated the test at rL321003 - please can you rebase and then regenerate the test with the update_llc_test_checks.py script to check the diff?

samparker updated this revision to Diff 127485.Dec 19 2017, 3:36 AM

Rebased and update the X86 test, plus I've added a couple of other ARM tests.

x86 codegen looks OK, one minor but no other comments from me.

test/CodeGen/X86/h-registers-1.ll
76 ↗(On Diff #127485)

For clarity please can you put this back on the previous line with the other arguments?

niravd accepted this revision.Dec 20 2017, 8:51 AM

LGTM modulo rksimon's comment.

This revision is now accepted and ready to land.Dec 20 2017, 8:51 AM

Will do, Thanks for the review!

Closed by commit rL321259: [DAGCombine] Improve ReduceLoadWidth for SRL (authored by sam_parker, committed by ). · Explain WhyDec 21 2017, 4:55 AM
This revision was automatically updated to reflect the committed changes.
samparker reopened this revision.Apr 6 2018, 7:55 AM
samparker added a subscriber: nemanjai.

I kindly got a reproducer from @nemanjai so now I've, hopefully, sorted out the issue that was causing big endian stage two builders to fail.

This revision is now accepted and ready to land.Apr 6 2018, 7:55 AM
samparker updated this revision to Diff 141344.Apr 6 2018, 7:57 AM

The trunc_i64_mask_srl test has been added. This example showed that I had a typo in my patch, I was checking the user of N and not 'N0'! So I've also renamed some variables to reduce the chance of possible confusion again.

nemanjai accepted this revision.Apr 7 2018, 5:05 AM

LGTM. I have tested this on PPC64BE bootstrap and the previous failures are gone. Also, can you please add the test case I sent you as a part of this patch? Clearly, we had missing coverage there.

Thanks. Yes, I will do.

Closed by commit rL329551: [DAGCombine] Improve ReduceLoad for SRL (authored by sam_parker, committed by ). · Explain WhyApr 9 2018, 1:19 AM
This revision was automatically updated to reflect the committed changes.