This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] use narrow load to avoid vector extract
ClosedPublic

Authored by spatel on May 25 2017, 4:38 PM.

Details

Summary

If we have (extract_subvector(load wide vector)) with no other users, that can just be (load narrow vector).

I need help to confirm that all of the test diffs are correct. When I saw how many AArch tests were changing I thought something went wrong, but on closer inspection, we just delete the '2' from all of those instructions. Hooray for mnemonics that actually make sense!

The memop chain updating is based on code that already exists multiple times in x86, so I think that should be pulled into a helper function as a follow-up. I wouldn't have gotten that sequence on my own.

Background: this is a potential improvement noticed via regressions caused by making x86's peekThroughBitcasts() not loop on consecutive bitcasts (see comments in D33137).

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.May 25 2017, 4:38 PM
niravd added inline comments.May 25 2017, 7:39 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14472 ↗(On Diff #100327)

The hasOneUse restriction seems overly conservative for most Targets and you've already the logic to deal with duplicated loads. This seems like a good place for "isSubVectorExtractFree".

spatel added inline comments.May 26 2017, 6:34 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14472 ↗(On Diff #100327)

Yes, I agree this is conservative. If it's ok, I'd prefer to make this a TODO comment in this patch, and then I'll follow-up with that small enhancement. I want to be extra cautious with memop transforms to make sure we don't hit any perf or correctness problems.

Just removing the one-use check (without checking extract cost) will result in another 31 regression test files changing, so I need to take a close look at all those diffs.

niravd edited edge metadata.May 26 2017, 6:43 AM

LGTM modulo minor comment.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14470 ↗(On Diff #100327)

You should use BaseIndexOffset for Offset extraction. This may be better deferred to another patch with a TODO as well.

14472 ↗(On Diff #100327)

Makes sense to me.

spatel added inline comments.May 26 2017, 6:47 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
14470 ↗(On Diff #100327)

Ah, right - I forgot about that API. I'm running up my TODO tab, but yes, let me tack on one more. :)

FWIW, this part of the code is also based on code from x86 lowering, so there may be more refactoring opportunity.

RKSimon accepted this revision.May 26 2017, 7:35 AM

LGTM

This revision is now accepted and ready to land.May 26 2017, 7:35 AM
This revision was automatically updated to reflect the committed changes.