Open a74nh opened 10 months ago
Tagging subscribers to this area: @dotnet/area-system-numerics See info in area-owners.md if you want to be subscribed.
Author: | a74nh |
---|---|
Assignees: | - |
Labels: | `area-System.Numerics` |
Milestone: | - |
This covers all the shift instructions in FEAT_SVE.
This list was auto generated from the C ACLE for SVE. The equivalent ACLE functions and corresponding Arm64 instructions are shown above each method.
Where possible, existing C# naming conventions have been matched.
Many of the C functions include predicate argument(s), of type svbool_t. These are missing from the C# method. It is expected that the Jit will create predicates where required, or combine with uses of conditionalSelect()
. For more discussion see #88140 comment.
Some of the generated methods have been rejected and appear commented at the bottom. These all match immediate variants of the SVE instructions and are not required. See #88140 comment
@kunalspathak @tannergooding @BruceForstall @TamarChristinaArm
If people are happy with the general format of this, then I'll start raising issues for the other entries. I currently have 27 issues for FEAT_SVE
, and 25 issues for FEAT_SVE2
and the other SVE features
This format looks fine, in general.
I currently have 27 issues for FEAT_SVE, and 25 issues for FEAT_SVE2 and the other SVE features
The one concern is that this is a lot of issues (and very high noise to data ratio, most of the comments in the list are irrelevant to API review), far more than we typically have to review an ISA and it may hinder the overall review process due to overhead.
If we can compress some together into 3-5 issues, it would probably be a lot nicer. If not, then we'll manage having many different issues and see how it well it does or doesn't work come API review.
This format looks fine, in general.
I currently have 27 issues for FEAT_SVE, and 25 issues for FEAT_SVE2 and the other SVE features
The one concern is that this is a lot of issues (and very high noise to data ratio, most of the comments in the list are irrelevant to API review), far more than we typically have to review an ISA and it may hinder the overall review process due to overhead.
If we can compress some together into 3-5 issues, it would probably be a lot nicer. If not, then we'll manage having many different issues and see how it well it does or doesn't work come API review.
How about the new version. I've grouped everything into Vector<T>
and removed all the comments.
My issue with doing it this way is that it may not be clear what is the mapping from C# to Arm64. Plus you can't see which varieties of T are supported (eg none of the bitwise methods support Float
or Double
). That may not be important for the API review. But, I've kept the expanded version below, so it's always easy to go from one to the other, and ignorable otherwise.
Using VectorT, results in 593 methods for FEAT_SVE. I've split that into 11 groups (of that, bitwise is by far the shortest).
Some methods will end of with multiple T types. For example, in SVE2
:
public static unsafe Vector<short> SubtractWideningLower(Vector<sbyte> left, Vector<sbyte> right);
public static unsafe Vector<int> SubtractWideningLower(Vector<short> left, Vector<short> right);
public static unsafe Vector<long> SubtractWideningLower(Vector<int> left, Vector<int> right);
etc...
becomes:
public static unsafe Vector<T> SubtractWideningLower(Vector<T2> left, Vector<T2> right);
Another way of doing this, could be:
public abstract class Sve : AdvSimd /// Feature: FEAT_SVE Category: bitwise
{
// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> And(Vector<T> left, Vector<T> right);
// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe T AndReduce(Vector<T> value);
// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> BitwiseClear(Vector<T> left, Vector<T> right);
// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> Cnot(Vector<T> value);
etc....
For those with multiple Ts:
// T: [short, sbyte], [int, short], [long, int], [ushort, byte], [uint, ushort], [ulong, uint]
public static unsafe Vector<T> SubtractWideningLower(Vector<T2> left, Vector<T2> right);
It depends whether that's useful for API review.
My issue with doing it this way is that it may not be clear what is the mapping from C# to Arm64.
We ultimately want this information, but in the actual implementation as part of the XML documentation comments.
The issue with it being in the main proposal API is it introduces a very high amount of noise compared to what API review needs to actually look at.
Plus you can't see which varieties of T are supported (eg none of the bitwise methods support Float or Double)
We do need to know what T are being supported.
Some example of some past ARM64 proposals include:
Each is a little bit unique, but in general they have related APIs grouped together and there is low noise ratio compared to the actual data that is important to API review.
Another way of doing this, could be:
I think something like this would be fine, the same goes for multiple T
. It allows us to have fewer issues, covering higher amounts of instructions, and with most of the text being information that is important to reviewing and approving the API surface.
Updated the API to use VectorT. Full versions included below it for completeness.
And another update to include the Arm instruction(s) at the end of the method signature.
eg:
/// T: byte, ushort, uint, ulong
public static unsafe Vector<T> ShiftLeftLogical(Vector<T> left, Vector<T> right); // LSL or LSLR (MOVPRFX)
Interestingly, LSLR
(Reversed logical shift left by vector) is given as a possible for this because if the destination and right
are in the same register, then you can emit LSLR dest, right, left
. They are included because I've got no way of automatically spotting them to remove (other than hardcoding).
The (MOVPRFX)
is added in brackets to show that you might have to emit a move prefix if the destination and left
are not using the same register. It doesn't indicate if the move prefix is connected to the LSL
or the LSLR
.
The *Reduce
APIs should probably be *Across
to match the existing convention. So AndAcross
, OrAcross
, XorAcross
, etc.
I'm fine with (and personally prefer) Nand
and Nor
(as well as Xnor
, if it ever exists). However, it is inconsistent with cases like AndNot
, OrNot
, etc. So we probably want to discuss in API review whether they should be NotAnd
, NotOr
, etc instead.
ShiftLeftLogical
and friends look to be missing some overloads. In particular they should have ShiftLeftLogical(Vector<T> value, byte count)
variants for LSL (immediate)
(predicated or unpredicated). There likely needs to be overloads taking signed types for the left and unsigned types for the right as well; otherwise users have to inconveniently insert casts to do logical shifting.
ShiftRightArithmeticDivide
should take a byte count
, not a ulong
, for the immediate (its constrained to [1, BitsPerElement]
). A short example of what it does, logically, would also be beneficial for API review. I believe it's this:
if (element < 0)
{
element += (1 << count) - 1;
}
return (element >> count);
Cnot
might end up having discussion on if its the right name. It exactly matches the instruction name, but doesn't necessarily make it clear to users that it's effectively doing (element == 0) ? 1 : 0
InsertIntoShiftedVector
would also benefit from an example. It is functionally doing elementN = elementN-1; ...; element0 = value
This issue has been marked needs-author-action
and may be missing some important information.
The
*Reduce
APIs should probably be*Across
to match the existing convention. SoAndAcross
,OrAcross
,XorAcross
, etc.
Done. Any changes like this will automatically filter into all the other APIs.
I'm fine with (and personally prefer)
Nand
andNor
(as well asXnor
, if it ever exists). However, it is inconsistent with cases likeAndNot
,OrNot
, etc. So we probably want to discuss in API review whether they should beNotAnd
,NotOr
, etc instead.
Updated to AndNot
, OrNot
to match.
ShiftLeftLogical
and friends look to be missing some overloads. In particular they should haveShiftLeftLogical(Vector<T> value, byte count)
variants forLSL (immediate)
(predicated or unpredicated). There likely needs to be overloads taking signed types for the left and unsigned types for the right as well; otherwise users have to inconveniently insert casts to do logical shifting.
Those exist in the rejected pile, eg:
/// public static unsafe Vector<sbyte> ShiftLeftLogical(Vector<sbyte> left, byte right); // svlsl[_n_s8]_m or svlsl[_n_s8]_x or svlsl[_n_s8]_z
As per this https://github.com/dotnet/runtime/issues/88140#issuecomment-1773112192, I was assuming this would be done as:
ShiftLeftLogical(left, Broadcast(right));
I've made this assumption throughout the APIs.
ShiftRightArithmeticDivide
should take abyte count
, not aulong
, for the immediate (its constrained to[1, BitsPerElement]
).
Oddly the ACLE uses a uint64_t but then has the comment:
imm2 minimum: 1; maximum: 8
or imm2 minimum: 1; maximum: 64
I'll change it to a byte. As with the previous discussion on immediates - how will this be constrained to the max value?
Examples added directed beneath the vectorT API block. I've taken a mix of the architecture spec, the ACLE and my own wording.
As per this https://github.com/dotnet/runtime/issues/88140#issuecomment-1773112192, I was assuming this would be done as
We typically allow overloads that take a scalar T or constant integer. This is done for convenience, so the user can clearly understand where there is an additional optimization possible, and because we have historically had cases where the scalar vs vector overload may be in different ISAs or have subtly different behavior.
We certainly don’t have to provide them and will optimize on the backend where possible regardless, but this would at least be consistent with what we already expose.
how will this be constrained to the max value?
We have a special ConstantExpected attribute that allows us to annotate the (contiguous) lower and upper bound.
You can see an example of this for the Shift APIs exposed for AdvSimd
If what the user specifies is actually out of range, we would typically throw as it being an invalid operation. If it is non constant, we have to emulate the functionality, typically by emitting a fallback or equivalent alternative instruction.
Shift by immediate is an interesting case here since it doesn’t, strictly speaking, have a defined behavior for Arm64 since out of range values cannot be encoded. The Shift by vector on the other hand treat it the same as repeatedly shifting by 1, count times, and therefore it becomes zero or allbitsset on overflow (depending on direction and if logical or arithmetic). So the logical thing to do is to throw it being invalid in that case
As per this #88140 (comment), I was assuming this would be done as
We typically allow overloads that take a scalar T or constant integer. This is done for convenience, so the user can clearly understand where there is an additional optimization possible, and because we have historically had cases where the scalar vs vector overload may be in different ISAs or have subtly different behavior.
We certainly don’t have to provide them and will optimize on the backend where possible regardless, but this would at least be consistent with what we already expose.
Happy to add them all back in. It is going to bloat the API by several hundred methods. Maybe they should go into a maybe bucket?
Maybe they should go into a maybe bucket.
That's reasonable to me 👍
Added the maybe bucket. This is the "Optional Entries:" in the vectorT section and with the rejected methods at the end of the full api.
Also, updated so that the VectorT API states if the original Arm instruction has a predicate. Eg:
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> And(Vector<T> left, Vector<T> right); // AND // predicated, MOVPRFX
The AND
instruction is predicated and so is marked with // predicated
. This info will most likely be needed for the review.
(op1, ulong imm2)
=> (value, [ConstantExpected] byte control)
Vector<T>
, not T
namespace System.Runtime.Intrinsics.Arm;
/// VectorT Summary
public abstract class Sve : AdvSimd /// Feature: FEAT_SVE Category: bitwise
{
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> And(Vector<T> left, Vector<T> right); // AND // predicated, MOVPRFX
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> AndAcross(Vector<T> value); // ANDV // predicated
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> AndNot(Vector<T> left, Vector<T> right); // NAND // predicated
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> BitwiseClear(Vector<T> left, Vector<T> right); // BIC // predicated, MOVPRFX
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> BooleanNot(Vector<T> value); // CNOT // predicated, MOVPRFX
/// T: float, double, sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> InsertIntoShiftedVector(Vector<T> left, T right); // INSR
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> Not(Vector<T> value); // NOT or EOR // predicated, MOVPRFX
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> Or(Vector<T> left, Vector<T> right); // ORR // predicated, MOVPRFX
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> OrAcross(Vector<T> value); // ORV // predicated
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> OrNot(Vector<T> left, Vector<T> right); // NOR or ORN // predicated
/// T: [sbyte, byte], [short, ushort], [int, uint], [long, ulong], [sbyte, ulong], [short, ulong], [int, ulong], [byte, ulong], [ushort, ulong], [uint, ulong]
public static unsafe Vector<T> ShiftLeftLogical(Vector<T> left, Vector<T2> right); // LSL or LSLR // predicated, MOVPRFX
/// T: byte, ushort, uint, ulong
public static unsafe Vector<T> ShiftLeftLogical(Vector<T> left, Vector<T> right); // LSL or LSLR // predicated, MOVPRFX
/// T: [sbyte, byte], [short, ushort], [int, uint], [long, ulong], [sbyte, ulong], [short, ulong], [int, ulong]
public static unsafe Vector<T> ShiftRightArithmetic(Vector<T> left, Vector<T2> right); // ASR or ASRR // predicated, MOVPRFX
/// T: sbyte, short, int, long
public static unsafe Vector<T> ShiftRightArithmeticForDivide(Vector<T> value, [ConstantExpected] byte control); // ASRD // predicated, MOVPRFX
/// T: byte, ushort, uint, ulong
public static unsafe Vector<T> ShiftRightLogical(Vector<T> left, Vector<T> right); // LSR or LSRR // predicated, MOVPRFX
/// T: [byte, ulong], [ushort, ulong], [uint, ulong]
public static unsafe Vector<T> ShiftRightLogical(Vector<T> left, Vector<T2> right); // LSR // predicated, MOVPRFX
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> Xor(Vector<T> left, Vector<T> right); // EOR // predicated, MOVPRFX
/// T: sbyte, short, int, long, byte, ushort, uint, ulong
public static unsafe Vector<T> XorAcross(Vector<T> value); // EORV // predicated
}
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics See info in area-owners.md if you want to be subscribed.