Page MenuHomePhabricator

[ARM] Fix PR36249: add expansion rule for VLD1d64 pseudo with reg WB
AbandonedPublic

Authored by thopre on Feb 6 2018, 7:32 AM.

Details

Summary

VLD1d64TPseudoWB_register and VLD1d64QPseudoWB_register are missing rules to expand them to non pseudo MIR. These are selected for ARMISD::VLD3_UPD/VLD4_UPD with v1i64 vectors in SelectVLD. This patch adds the necessary mapping to VLD1d64Twb_register and VLD1d64Qwb_register respectively. As for the _fixed variant, the cost of these is bumped for unaligned access.

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Feb 6 2018, 7:32 AM

Hi all,

Ping? Best regards,

Thomas

srhines added a subscriber: srhines.

This is blocking https://reviews.llvm.org/D42970, which we would also like to see fixed. This patch seems fairly straightforward to me, although I don't want to approve an ARM backend change.

Hi Thomas,

Please add some checks into the test case.

Hi Sam,

Code generation for these pseudo is already checked by the follow-on patch in https://reviews.llvm.org/D42970 (the very testcase that triggered the need for this present fix). I feel that adding checks here again would mean more testcases need to be updated in case code generation for these pseudo instructions needs to be changed. I also feel it brings a nice separation of concern: one test checks code generation, one checks correct expansion / absence of assert failure.

If you disagree let me know and I'll add the check right away.

If these two patches are solving the same issue, then they should be submitted for review as a single patch. As I understand, you could do that and not include this mir test and still have the functionality fully tested? This would make less work for you and the reviewer :)
Otherwise, I would say that any codegen tests should always have a test for what code is being generated, because its easy to add a check. If a test is added purely to trigger an assert or timeout, it is usually explicitly stated in the test case.

thopre abandoned this revision.Feb 23 2018, 4:54 AM

This has been merged into https://reviews.llvm.org/D42970, abandoning this patch.