Page MenuHomePhabricator

[DAGCombine] visitEXTRACT_SUBVECTOR - 'little to big' extract_subvector(bitcast()) support
ClosedPublic

Authored by spatel on Jun 26 2019, 5:26 AM.

Details

Summary

This moves the X86 specific transform from rL364407 into DAGCombiner to generically handle 'little to big' cases (e.g. extract_subvector(v2i64 bitcast(v16i8))), this allows us to remove both the x86 implementation and the aarch64 bitcast(extract_subvector(bitcast())) combine (note none of the regressions are due to removing this).

We see a number of regressions which is why I went for making it x86 in the first place:

AArch64's poor handling of shuffle(bitcast(),bitcast()) cases - AArch64ISD::DUPLANE cases in particular as well as "extract high" patterns.

The merge-store.ll change is interesting - AArch64TargetLowering::allowsMisalignedMemoryAccesses permits 'fast' unaligned v2i64 stores in all cases which is being exposed by removing the bitcast.

Most likely we need to address these issues before this patch can proceed - I'm happy to help if someone can suggest what is the most critical to fix first.

Diff Detail

Event Timeline

RKSimon created this revision.Jun 26 2019, 5:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 5:26 AM

It looks like the arm64-neon-2velem.ll regressions are a shuffle lowering issue, yes; we're creating a DUPLANE where the operand is an extract_subvector, and it doesn't simplify.

The failure to match fcvtl2 probably is just a matter of fixing the pattern in the .td file: it explicitly checks for an operation where the operand is an extract_subvector.

Not sure what the right resolution is for allowsMisalignedMemoryAccesses; we currently only use FeatureSlowMisaligned128Store for certain Exynos targets. Probably not too important.

spatel commandeered this revision.Dec 18 2019, 8:17 AM
spatel edited reviewers, added: RKSimon; removed: spatel.

Commandeering to rebase. D71515 / rG5e5e99c041e4 removes one of the problems, but we probably need at least 1 other fix.

spatel updated this revision to Diff 234545.Dec 18 2019, 8:21 AM

Patch updated:
Rebased to current trunk. The previous rev did not apply cleanly anymore. Some AArch diffs are gone as expected. There are x86 test diffs now, but these appear neutral or better AFAICT.

spatel updated this revision to Diff 235048.Dec 22 2019, 6:31 AM
spatel added a reviewer: craig.topper.

Patch updated:
Rebased after AArch64 change - rG0b38af89e2c0
I think that was the last known regression that required fixing.

RKSimon accepted this revision.Dec 22 2019, 9:43 PM

LGTM - thanks for completing this

This revision is now accepted and ready to land.Dec 22 2019, 9:43 PM
This revision was automatically updated to reflect the committed changes.