[PATCH] D70456: [Matrix] Add first set of matrix intrinsics and initial lowering pass.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/docs/LangRef.rst:14451
+
+      declare vectorty @llvm.matrix.multiply.*(vectorty %A, vectorty %B, i32 <M>, i32 <N>, i32 <K>)
+
----------------
LuoYuanke wrote:
> fhahn wrote:
> > LuoYuanke wrote:
> > > I have a question for the shape propagation. What if there is some conflict of the shape.  Take below code for example. The shape of matrix A is defined both by load and multiply. What is shape of matrix C and D? 4x4 or 5x5?
> > > 
> > > ```
> > > A = llvm.matrix.columnwise.load(ptr, stride, 5, 5);
> > > B = llvm.matrix.columnwise.load(ptr, stride, 5, 5);
> > > C = A + B
> > > llvm.matrix.multiply(A, B, 4, 4, 4);
> > > llvm.matrix.multiply(A, B, 5, 5 ,5);
> > > D = A - B
> > > 
> > > ```
> > I think the example above is not entirely valid as the loaded values are of type <25 x double>  and the first multiply would take arguments with <16 x double>. I've put up D77129 to update the verifier to reject such IR.
> > 
> >  However a conflict between shapes can occur, if the number of elements matches, as in the example below. In cases where the shape information inferred for an operand does not match the shape required at a use, we embed the operand into a flat vector and extract vectors of the required shape for that use (it is done in getMatrix  https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp#L331)
> > 
> > ```
> > A = llvm.matrix.columnwise.load(ptr, stride, 2, 3); // Shape of A = 2 x 3
> > llvm.matrix.multiply(A, A, 2, 3, 2); // first operand requires  2 x 3 - use A directly
> >                                      // second operand requires  3 x 2 - cannot use A directly; embedded A in flat vector and extract columns assuming 3 x 2.
> > ```
> > 
> > The specification of the intrinsics does not mention shape propagation and the shape imposed by the intrinsics only applies to the uses at the intrinsic calls. Shape propagation is only used for optimization purposes to better split up instructions that do not require shape info.
> > 
> > Does that make sense? It might be desirable to add an option to check for conflicts.
> I prefer to be more strict for the syntax.  The example that you provide is also illegal to me. The matrix should be fixed when matrix is defined.  Maybe we can have some reshape operator to explicitly reshape the matrix.
> 
> 
> ```
> C = llvm.matrix.reshape(A, row, col);
> ```
> 
> ```
> A = llvm.matrix.columnwise.load(ptr, stride, 2, 3); // Shape of A = 2 x 3
> llvm.matrix.multiply(A, A, 2, 3, 2); // first operand requires  2 x 3 - use A directly
>                                                      // second operand requires  3 x 2 - illegal.
> ```
> I prefer to be more strict for the syntax. The example that you provide is also illegal to me. The matrix should be fixed when matrix is defined. Maybe we can have some reshape operator to explicitly reshape the matrix.

Unfortunately I think without a matrix type on the IR level, such a stricter syntax won't really be feasible. The intrinsics operate on arbitrary IR vectors and the operands/results can also be used by other instructions. At the moment, the matrix intrinsics and regular IR instructions interact nicely, I think exactly because they operate exclusively on flat vectors, with the intrinsics having some additional information. But the intrinsics themselves produce a flat vector again. This keeps the spec simple and also has the advantage of giving the optimizer freedom to do shape related optimizations.

A restriction as you mentioned would have to be enforced on the frontend side I think. For example, there should be no shape mismatches generated by Clang with our proposal/patches.

It might be good to add an option to the lowering pass to turn shape mismatches into error to verify a frontend does indeed not introduce conflicts. 

What you you think?






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70456/new/

https://reviews.llvm.org/D70456