Implement bmask in AArch64#3384
Conversation
|
We should mention that this pull request is the AArch64 implementation of issue #1429. I will look at the changes; in the meantime, if someone else wants to as well, please, do. |
| to_bits: cmp::min(to_bits, 64), | ||
| }); | ||
| } else { | ||
| // Smaller integers/booleans are stored with high-order bits undefined, so we can |
There was a problem hiding this comment.
I am afraid that this is not true for vector types; e.g. consider an operation from B8X8 to B16X8.
Unless both the input and the output have the same lane sizes (the number of lanes is always the same, as specified by the operation descriptions), you will have to write much more complicated logic, and will probably be unable to merge the scalar and vector code paths, as you do now.
There was a problem hiding this comment.
Right, I didn't really prepare this for those types (non 128 bit vectors). I haven't really seen them used anywhere so far.
Would it be acceptable to assert on 128bit vectors for now?
There was a problem hiding this comment.
No, just keep the block (lines 1864 - 1869) that you have removed. Of course, you will need to change the condition - I suppose to from_ty.is_vector() && (from_bits != 128 || from_bits != to_bits).
IMHO assertions should be used for verifying invariants guaranteed by the IR descriptions, e.g. checking that the number of input lanes is equal to the number of output lanes. That is the approach I have used in my recent changes.
BTW the vector versions of all those operations except Bmask pretty much do not make sense unless you are dealing with non-128-bit vectors.
There was a problem hiding this comment.
Even after your latest changes, this comment should be changed to Smaller scalar integers/booleans are....
| signed: true, | ||
| from_bits, | ||
| to_bits, | ||
| from_bits: cmp::min(from_bits, 64), |
There was a problem hiding this comment.
Given that to_bits > from_bits, we know that from_bits <= 64, so there is no need to calculate a minimum here.
| // Duplicate the bottom register to the top, for B128 they are always equal. | ||
| // If we get a I128 here, it means that we are lowering a `bmask`, in which | ||
| // case we also need to duplicate the bottom register. | ||
| assert!(op == Opcode::Bextend || op == Opcode::Bmask); |
There was a problem hiding this comment.
Remove this assertion - Breduce from B128 to B128 and Ireduce from I128 to I128 are also valid operations.
|
|
||
| if to_ty == B128 || to_ty == I128 { | ||
| // Duplicate the bottom register to the top, for B128 they are always equal. | ||
| // If we get a I128 here, it means that we are lowering a `bmask`, in which |
There was a problem hiding this comment.
We could also be lowering Ireduce - see below.
There was a problem hiding this comment.
Now that I think about it, Ireduce from I128 to I128 needs completely different treatment - the upper destination register is not a copy of the lower source register. Perhaps handling all those operations in a single place is not the best approach, and keeping Breduce and Ireduce separate (as in the original code) is better.
There was a problem hiding this comment.
The docs state that ireduce requires that the output type is smaller than the input type, so at most we only ever have a i64 output (if we get a i128 input). I've checked and the verifier enforces this. breduce has the same language in the docs.
There was a problem hiding this comment.
Well, then there is a contradiction because the documentation right now states:
The result type must have the same number of vector lanes as the input, and each lane must not have more bits that the input lanes.
In other words, there are two options - either the same number of bits per lane, or fewer. If indeed the verifier enforces the second option, then do you mind making the description less ambiguous? Same for Breduce.
| // If we get a I128 here, it means that we are lowering a `bmask`, in which | ||
| // case we also need to duplicate the bottom register. | ||
| assert!(op == Opcode::Bextend || op == Opcode::Bmask); | ||
| ctx.emit(Inst::gen_move(dst.regs()[1], dst.regs()[0].to_reg(), to_ty)); |
There was a problem hiding this comment.
I feel like this should be calculated in parallel with the bottom half, at the expense of a bit of code duplication, especially in the case of from_bits == to_bits.
|
With the ISLE transition in full swing now, this PR should migrate all its changes to the ISLE code (or be closed and replaced with a new PR). |
|
I'll close this for now, and try again on the ISLE machinery if I have time. Thanks |
Hey, this PR Implements
bmaskfor all types in the AArch64 backend, as well as fixes forireduce,breduceandbextendfor some types.We have to make some changes in the trampoline to make bool args with size larger than 1 work.