Page MenuHomePhabricator

Expand masked mem intrinsics correctly wrt big-endian
ClosedPublic

Authored by markus on Jan 15 2021, 4:37 AM.

Details

Summary

Need to take endianness into account when doing vector to scalar casts such as

%bc = bitcast <8 x i1> %v to i8

Companion review for https://reviews.llvm.org/D94867
Upload in response to https://lists.llvm.org/pipermail/llvm-dev/2021-January/147862.html
Attempting to document the actual memory layout rules for vectors in https://reviews.llvm.org/D94964

Diff Detail

Event Timeline

markus created this revision.Jan 15 2021, 4:37 AM
markus requested review of this revision.Jan 15 2021, 4:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 4:37 AM

Thanks for the patch. There is a patch to make MVE consistent with the rest of MVE in D94867. This will need rebasing on top of that, with update tests to make the two consistent again.

llvm/lib/Transforms/Scalar/ScalarizeMaskedMemIntrin.cpp
332–333

It's a shame we can't just use extract elements - that would simplify this a lot!

Thanks for the patch. There is a patch to make MVE consistent with the rest of MVE in D94867. This will need rebasing on top of that, with update tests to make the two consistent again.

Thank you for looking into this.

llvm/lib/Transforms/Scalar/ScalarizeMaskedMemIntrin.cpp
332–333

It's a shame we can't just use extract elements - that would simplify this a lot!

I fully agree.

markus updated this revision to Diff 320086.Jan 29 2021, 2:47 AM
markus retitled this revision from [WIP]Expand masked mem intrinsics correctly wrt big-endian to Expand masked mem intrinsics correctly wrt big-endian.
markus edited the summary of this revision. (Show Details)
markus added reviewers: greened, bjope.
bjope updated this revision to Diff 320380.Jan 31 2021, 3:12 PM

Add tests using big endian datalayout.

Also updated test checks in the thumb codegen test cases that is impacted by
this change. Not sure if this patch should be parent or child to D94867 (as
both patches need to land at the same time to not break big-endian ARM.

dmgreen edited reviewers, added: dmgreen; removed: greened.Feb 1 2021, 12:14 AM
dmgreen accepted this revision.Feb 8 2021, 3:49 AM

OK. Thanks for the commit. This LGTM.

Are you happy for me to commit this and D94867 together, to keep them in-sync?

This revision is now accepted and ready to land.Feb 8 2021, 3:49 AM
bjope added a comment.Feb 8 2021, 11:57 AM

OK. Thanks for the commit. This LGTM.

Are you happy for me to commit this and D94867 together, to keep them in-sync?

(answering on behalf of Markus who will be out-of-office for a couple of weeks)

That sounds great! Thanks for all the help with this problem.

This revision was landed with ongoing or failed builds.Feb 11 2021, 1:00 AM
This revision was automatically updated to reflect the committed changes.