This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Make a BE predicate bitcast consistent with the rest of llvm
ClosedPublic

Authored by dmgreen on Jan 16 2021, 8:57 AM.

Details

Summary

We were storing predicate registers, such as a <8 x i1>, in the opposite order to how the rest of llvm expects. This actually turns out to be correct for the one place that usually uses it - the ScalarizeMaskedMemIntrin pass, but only because the pass was incorrect itself. This fixes the order so that bits are stored in the opposite order and bitcasts work as expected. This allows the Scalarization pass to be fixed, as in https://reviews.llvm.org/D94765.

Diff Detail

Event Timeline

dmgreen created this revision.Jan 16 2021, 8:57 AM
dmgreen requested review of this revision.Jan 16 2021, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2021, 8:57 AM
markus added a subscriber: markus.Jan 17 2021, 9:13 PM

We were storing predicate registers, such as a <8 x i1>, in the opposite order to how the rest of llvm expects.

It should be mentioned that it is, at least to me, unclear what llvm expects wrt this and as far as I know it is not documented anywhere. Simple experiment suggest that bit order is reversed for big endian targets

define i8 @foo() {
entry:
  %v = insertelement <8 x i1> zeroinitializer, i1 true, i8 0
  %bc = bitcast <8 x i1> %v to i8
  ret i8 %bc
}
$ llc -O3 bitcast.ll --mtriple arm -o -     # lsb is set in scalar
$ llc -O3 bitcast.ll --mtriple armeb -o -     # msb is set in scalar

with similar results for mips (big-endian) and amd64 (little-endian).

So before we go ahead an commit anything this should probably be clarified. I tried raising the issue on llvm-dev without much definitive response https://lists.llvm.org/pipermail/llvm-dev/2021-January/147725.html

We were storing predicate registers, such as a <8 x i1>, in the opposite order to how the rest of llvm expects.

It should be mentioned that it is, at least to me, unclear what llvm expects wrt this and as far as I know it is not documented anywhere. Simple experiment suggest that bit order is reversed for big endian targets

define i8 @foo() {
entry:
  %v = insertelement <8 x i1> zeroinitializer, i1 true, i8 0
  %bc = bitcast <8 x i1> %v to i8
  ret i8 %bc
}
$ llc -O3 bitcast.ll --mtriple arm -o -     # lsb is set in scalar
$ llc -O3 bitcast.ll --mtriple armeb -o -     # msb is set in scalar

with similar results for mips (big-endian) and amd64 (little-endian).

Yes, but that is base ARM and it is only MVE that is incorrect. You can see here that things were inconsistent, which is what this patch is fixing:
https://godbolt.org/z/M8Y6dv

So before we go ahead an commit anything this should probably be clarified. I tried raising the issue on llvm-dev without much definitive response https://lists.llvm.org/pipermail/llvm-dev/2021-January/147725.html

It comes from https://reviews.llvm.org/D42100#992315. This patch just brings MVE inline with what the rest of llvm expects.

Yes, but that is base ARM and it is only MVE that is incorrect. You can see here that things were inconsistent, which is what this patch is fixing:
https://godbolt.org/z/M8Y6dv

Yes. I agree. As long was we know which is the right behavior.

It comes from https://reviews.llvm.org/D42100#992315. This patch just brings MVE inline with what the rest of llvm expects.

Thank you! I have been trying to find where this decision was taken. But now that we know that shouldn't we make an effort to document this in the lang-ref document (or is it already and simply couldn't find it)? It seems like something that is quite fundamental to be aware of and not at all obvious.

This revision is now accepted and ready to land.Feb 8 2021, 1:16 AM