Page MenuHomePhabricator

[Verifier] Verify matrix dimensions operands match vector size.
ClosedPublic

Authored by fhahn on Mar 31 2020, 3:07 AM.

Details

Summary

This patch adds checks to the verifier to ensure the dimension arguments
passed to the matrix intrinsics match the vector types for their
arugments/return values.

Diff Detail

Event Timeline

fhahn created this revision.Mar 31 2020, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 3:07 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
anemet accepted this revision.Mar 31 2020, 9:33 AM

Nice, LGTM!

This revision is now accepted and ready to land.Mar 31 2020, 9:33 AM
This revision was automatically updated to reflect the committed changes.
SjoerdMeijer added inline comments.
llvm/lib/IR/Verifier.cpp
4805

Quick query on this and the semantics:

declare vectorty @llvm.matrix.multiply.*(vectorty %A, vectorty %B, i32 <OuterRows>, i32 <Inner>, i32 <OuterColumns>)

do we expect the element types of vectors %A and %B to be same, and do we need to check this?

fhahn marked an inline comment as done.Jul 8 2020, 3:19 AM
fhahn added inline comments.
llvm/lib/IR/Verifier.cpp
4805

Yes, the element types of all types must match currently, but I think it is neither checked in the verifier nor explicit in the LangRef.

To generate code for llvm.aarch64.neon.udot & co, there probably needs to be a way to have different element type widths for result and source operands.

SjoerdMeijer added inline comments.Jul 8 2020, 3:27 AM
llvm/lib/IR/Verifier.cpp
4805

Yes, the element types of all types must match currently, but I think it is neither checked in the verifier nor explicit in the LangRef.

I started looking at the matrix support, getting up to speed with it, and this is where I started and the first thing I noticed. Was just asking about that here as a sanity check. I wouldn't mind putting up a patch for that if that's helpful. Probably the least we can do for not is to check if we are not mixing integers and float types, and then we also need to add that to LangRef and be explicit about that.

fhahn marked an inline comment as done.Jul 8 2020, 3:53 AM
fhahn added inline comments.
llvm/lib/IR/Verifier.cpp
4805

I wouldn't mind putting up a patch for that if that's helpful.

That would be great. I think things will fall apart/miscompile if the element types differ at the moment.

SjoerdMeijer added inline comments.Jul 8 2020, 9:15 AM
llvm/lib/IR/Verifier.cpp
4805

cool, will do.

FYI: I started with committing a NFC patch adding some more negative tests: rG0fc17e9edc8f