This tightens the matrix intrinsic definitions in LLVM LangRef and adds corresponding checks to the IR Verifier.
Details
Diff Detail
Event Timeline
Thanks for improving the LangRef and verifier. I am not entirely sure about referring to the matrixes as linearized (see inline comment), otherwise looks great.
llvm/docs/LangRef.rst | ||
---|---|---|
15498–15499 | When I read linearized here, I thing about https://en.wikipedia.org/wiki/Linearization , so there might be potential for confusion. It might be worth defining exactly what we mean be embedding here, then further uses should be un-ambigous: the columns of a matrix R x C are embedded into a vector such that the elements of subsequent columns are adjacent in the vector. Or more formally element I of column J is at index J * R + I in the vector (with indices starting at 0) |
llvm/docs/LangRef.rst | ||
---|---|---|
15498–15499 | Yep, thanks. I was looking how to rephrase "embedded", but agree that "linearization" is perhaps equally vague, so yes this is the best we can do:
Will go for that one. |
llvm/docs/LangRef.rst | ||
---|---|---|
15498–15499 | It would also be good to say that layout defaults to column major currently. It can be changed globally during the lowering to row-major as well, but we probably do not want to mention actual pass specifics here. |
llvm/docs/LangRef.rst | ||
---|---|---|
15578 | I am actually now also interested in defining %Stride better. Using our new definition:
From the description of %Stride it follows that: %Stride = ( (j+1) * R + 0) - (j * R + 0) => %Stride = R So double checking: we can simply the description of %Stride just by saying it is equal to the number of rows, is that correct? |
llvm/docs/LangRef.rst | ||
---|---|---|
15578 | Stride can be > the number of rows. For example, if you want to load a 2x2 sub-matrix from a 4x4 matrix, you would use `llvm.matrix.column.major.load(%start, 4, false, 2, 2), where %start points to the first element of the sub-matrix. The function to compute column addresses has an extensive comment about how things work: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp#L92 It boils down to something like: the start address of column I in memory is computed as getelementptr %Start, I * Stride. |
llvm/docs/LangRef.rst | ||
---|---|---|
15578 | Ah yes, thanks, I see now. I will add this, and we have at least one more condition, Stride >= Rows, to add to the verifier. |
llvm/docs/LangRef.rst | ||
---|---|---|
15578 | ignore:
%Stride is not an immediate. |
llvm/docs/LangRef.rst | ||
---|---|---|
15578 | yes, the stride can be an arbitrary value. In some (probably most) it will be a ConstantInt, so it might be worth just checking for ConstantInt. |
As discussed:
- removed "linearization" and replaced it with the explanation how matrices are laid out in vectors.
- Similarly, spent some words how Stride is used/calculated
- added a check for Stride >= Rows.
LGTM, thanks! Some optional nits related to wording inline (I think it would be good to start the sentences for the arguments with a The).
llvm/docs/LangRef.rst | ||
---|---|---|
15502 | maybe something like in the corresponding vector instead of in its vector, where it might be a little unclear what its refers to. | |
15527–15531 | The first ..? | |
15554–15559 | The first... and the second ...? | |
15558 | must all have ? | |
15588–15596 | The first...? | |
15589 | The second? | |
15592 | The third | |
15628–15636 | The first argument %In is a vector? | |
15629 | The second argument %Ptr is a pointer to the? | |
15630 | The third? | |
15633 | The fourth? | |
15634 | The arguments? | |
llvm/lib/IR/Verifier.cpp | ||
5069 | It would be good to be consistent with the capitalization/puncation with the existing message at 5073 or update the message there. Also, it might be good to include vector element type in the message, as in the message for Op0. |
Had to revert this because somehow I missed a few failing regression test. Regarding this, wanted to check one thing @fhahn.
In test/Transforms/LowerMatrixIntrinsics/strided-store-i32.ll, we have for example:
call void @llvm.matrix.column.major.store(<6 x i32> %in, i32* %out, ..
And I am thinking that this should be:
call void @llvm.matrix.column.major.store(<6 x i32> %in, <6 x i32>* %out, ..
This would match the intrinsic description which says that the second argument should be a pointer to the first matched type:
[llvm_anyvector_ty, LLVMAnyPointerType<LLVMMatchType<0>>,
Agree, or am I perhaps missing something? If not and you agree, I will modify the loads/store tests in test/Transforms/LowerMatrixIntrinsics/ before recommitting this.
I think the intrinsic definition is wrong here (and it also seems like the LLVMAnyPointerType does not actually result in the expected check). I think should pass a pointer to the element type directly (rather than a pointer to a vector), because if stride > R we would access elements outside of the vector. Granted, nothing should really rely on the pointer type for aliasing purposes and so on, but it seems misleading to pass in e.g. <6 x i32>* and then access elements other than the first 6 i32, e.g. due to the stride being 10.
I missed that in the adjustments of the langref, I think we specify that %Ptr needs to be a pointer to the element type of the vector.
I think the intrinsic definition is wrong here (and it also seems like the LLVMAnyPointerType does not actually result in the expected check). I think should pass a pointer to the element type directly (rather than a pointer to a vector), because if stride > R we would access elements outside of the vector. Granted, nothing should really rely on the pointer type for aliasing purposes and so on, but it seems misleading to pass in e.g. <6 x i32>* and then access elements other than the first 6 i32, e.g. due to the stride being 10.
I missed that in the adjustments of the langref, I think we specify that %Ptr needs to be a pointer to the element type of the vector.
Okay, cool, that's actually what I was expecting.
Just checking that I don't get into your way, shall I prepare a patch for that?
I don't mind. The current patch is reverted at the moment, right? So it might be easiest to just fold those small changes directly into it? Otherwise I can do it as follow-up once the current patch lands.
I don't mind. The current patch is reverted at the moment, right? So it might be easiest to just fold those small changes directly into it? Otherwise I can do it as follow-up once the current patch lands.
Yep, it's reverted because we have different ptr types (ptr to vector vs. ptr to scalar) in different tests now, and these new Verifier checks don't like that.
As I need to make changes to this patch anyway in order to recommit it, you're right that I can just fold it into this, that's the easiest I guess so will do that then (and will put that up for review).
When I read linearized here, I thing about https://en.wikipedia.org/wiki/Linearization , so there might be potential for confusion.
It might be worth defining exactly what we mean be embedding here, then further uses should be un-ambigous: the columns of a matrix R x C are embedded into a vector such that the elements of subsequent columns are adjacent in the vector. Or more formally element I of column J is at index J * R + I in the vector (with indices starting at 0)