This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add MVE_TwoOpPattern. NFC
ClosedPublic

Authored by dmgreen on Jul 6 2020, 5:46 AM.

Details

Summary

This commons out a chunk of the different two operand MVE patterns into a single helper multidef. Or technically two multidef patterns so that the Dup qr patterns can also get the same treatment. This is most of the two address instructions that we have some codegen pattern for (not ones that we select purely from intrinsics). It does not include shifts, which are more spread out and will need some extra work to be given the same treatment.

Diff Detail

Event Timeline

dmgreen created this revision.Jul 6 2020, 5:46 AM
SjoerdMeijer accepted this revision.Jul 16 2020, 2:16 AM

Looks like a good refactoring to me.

This revision is now accepted and ready to land.Jul 16 2020, 2:16 AM
This revision was automatically updated to reflect the committed changes.

This is causing runtime failures in at least ffmpeg, libvpx and dav1d, when targeting armv7-w64-mingw32, so it doesn't seem to be NFC as intended. I'll try to get a small reproducer later today...

https://martin.st/temp/dav1d-obu.ll - the output from llc dav1d-obu.ll shows a difference.

We're also seeing runtime failures in Halide (SIGILL). Either reverting or having a quick forward fix would be very much appreciated.

Oh. I must have missed a requires clause I suspect. I'll take a look now, and revert if I can't find the problem.

Yeah OK. This should be simple enough. It's missing the predicate off VMUL_qr. I'll just run a quick check-all and submit the fix.

OK. Hopefully fixed in 411eb87c7962ec817ab6bf7aa3c737a3159d2d4e. Thanks for the report/reproducer, and sorry I didn't catch it earlier.

Please let me know if any problems persist. I looked through the other cases and they all look like they are guarded properly.

OK. Hopefully fixed in 411eb87c7962ec817ab6bf7aa3c737a3159d2d4e. Thanks for the report/reproducer, and sorry I didn't catch it earlier.

Please let me know if any problems persist. I looked through the other cases and they all look like they are guarded properly.

Thanks for the quick fix! I'll know by tomorrow if all issues I ran into were fixed.

dmajor added a subscriber: dmajor.Jul 22 2020, 4:35 PM

We just noticed this in Firefox as well, and I confirmed that 411eb87c7962ec817ab6bf7aa3c737a3159d2d4e fixes the issue for us. Thanks for the fast response!

OK. Hopefully fixed in 411eb87c7962ec817ab6bf7aa3c737a3159d2d4e. Thanks for the report/reproducer, and sorry I didn't catch it earlier.

Please let me know if any problems persist. I looked through the other cases and they all look like they are guarded properly.

Thanks for the quick fix! I'll know by tomorrow if all issues I ran into were fixed.

All the cases I saw seem to be fixed now - thanks!

Thank you very much for the quick patch! Confirming Halide is fixed too.