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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
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.