Page MenuHomePhabricator

[Matrix] Tighten LangRef definitions and Verifier checks.
ClosedPublic

Authored by SjoerdMeijer on Jul 9 2020, 5:47 AM.

Details

Summary

This tightens the matrix intrinsic definitions in LLVM LangRef and adds corresponding checks to the IR Verifier.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Jul 9 2020, 5:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
fhahn added a comment.Jul 9 2020, 8:03 AM

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)

SjoerdMeijer marked an inline comment as done.Jul 9 2020, 9:20 AM
SjoerdMeijer added inline comments.
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:

Or more formally element I of column J is at index J * R + I in the vector (with indices starting at 0)

Will go for that one.

fhahn added inline comments.Jul 9 2020, 9:23 AM
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.

SjoerdMeijer marked an inline comment as done.Jul 9 2020, 9:49 AM
SjoerdMeijer added inline comments.
llvm/docs/LangRef.rst
15578

I am actually now also interested in defining %Stride better. Using our new definition:

For a R x C matrix, element i of column j is at index j * R + i in its vector, with indices starting at 0.

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?

fhahn added inline comments.Jul 9 2020, 10:07 AM
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.

SjoerdMeijer marked an inline comment as done.Jul 10 2020, 1:14 AM
SjoerdMeijer added inline comments.
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.

SjoerdMeijer marked an inline comment as done.Jul 10 2020, 1:36 AM
SjoerdMeijer added inline comments.
llvm/docs/LangRef.rst
15578

ignore:

and we have at least one more condition, Stride >= Rows, to add to the verifier.

%Stride is not an immediate.

fhahn added inline comments.Jul 10 2020, 1:46 AM
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.

Removed unnecessary extra newline.

fhahn accepted this revision.Jul 10 2020, 9:22 AM

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

The first ..?

15554

The first... and the second ...?

15558

must all have ?

15588

The first...?

15589

The second?

15592

The third

15628

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.

This revision is now accepted and ready to land.Jul 10 2020, 9:22 AM

Thanks for reviewing, and I will make those changes before committing.

This revision was automatically updated to reflect the committed changes.
SjoerdMeijer added a comment.EditedJul 13 2020, 2:27 AM

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.

fhahn added a comment.Jul 13 2020, 2:45 AM

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?

fhahn added a comment.Jul 13 2020, 3:12 AM

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.

SjoerdMeijer added a comment.EditedJul 13 2020, 3:19 AM

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).

fhahn added a comment.Jul 13 2020, 3:28 AM

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).

Great, thanks!