This is an archive of the discontinued LLVM Phabricator instance.

[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!

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.

This revision was automatically updated to reflect the committed changes.
gchatelet added inline comments.
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3841

I stumbled upon this code while debugging a regression in D81379.
AFAICT ShAmt (ShiftAmount?) can be a non power of two, especially when using the sanitizers.
For instance, ShAmt can be 48, it is a round number of byte but not a valid alignment 48/8==6.

I'm not quite sure what is the expected fix so I'm commenting here to get some feedback.
It seems to me that it should be the natural alignment for ExtVT possibly constrained by the original LoadN alignment.
Something along those lines:

if (ShAmt) {
  assert(ShAmt % 8 == 0 && "ShAmt is byte offset");
  const unsigned ByteShAmt = ShAmt / 8;
  const Align LDSTAlign = LDST->getAlign();
  const Align NarrowAlign = commonAlignment(LDSTAlign, ByteShAmt);
  if (!TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), MemVT,
                              LDST->getAddressSpace(), NarrowAlign.value(),
                              LDST->getMemOperand()->getFlags()))
    return false;
}

Would that make sense?

Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2020, 1:45 PM
samparker marked an inline comment as done.Jun 10 2020, 1:51 AM
samparker added inline comments.
llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3841

I'm not sure about the assert - as I remember, this function should just return false for anything that wouldn't be a legal load, whereas it's perfectly legal to have a shift of, say, 48 somewhere in the DAG. Other than that, looks like a good fix to me.