Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DaphneDSL: Matrix literals #358

Closed
RetoKrummenacher opened this issue May 6, 2022 · 7 comments · Fixed by #387
Closed

DaphneDSL: Matrix literals #358

RetoKrummenacher opened this issue May 6, 2022 · 7 comments · Fixed by #387
Assignees

Comments

@RetoKrummenacher
Copy link

Presently, it is not possible to generate a matrix with desired values. Like a magic square with predefined values at desired position. Currently, such a matrix needs to be created with a series of fill, cbind and rbind operations.

There are several possible ways to provide this:

  1. Similar to Matlab by just writing values in brackets
  2. Similar to R by creating a one dimensional matrix and reshaping it to the desired dimensions.
# matlab
m = [17 24 1 8 15, 23 57 14 16];
# R
v = c(17 2 4 1 8 15 23 5 7 14 16);
m = reshape(m,5,2)
# or with matrix command
m = matrix(17 2 4 1 8 15 23 5 7 14 16); 

This would be useful for example when solving given small systems of linear equations. Then the matrix can be created directly in DaphneDSL instead of importing it from a file.

@pdamme
Copy link
Collaborator

pdamme commented May 12, 2022

Thanks for suggesting this, @RetoKrummenacher. This is a great idea (and actually has been on my personal wish list for a while). We'll definitely implement this feature.

@akroviakov
Copy link
Collaborator

Assigned to @akroviakov.

@pdamme
Copy link
Collaborator

pdamme commented May 13, 2022

@akroviakov: A few hints on the most important points (as discussed today):

  • extension of the DaphneDSL grammar to support matrix literals
  • DaphneDSL parser should
    1. create a runtime DenseMatrix object (e.g., a column matrix, can the reshaped by reshape(r, c, [...]), and
    2. generate a MatrixConstantOp (to be created) which gets the pointer to the DenseMatrix as a uint64_t
  • a kernel for MatrixConstantOp, which simply returns the reinterpret_cast'ed pointer as a DenseMatrix
  • registration of the kernel for typical value types
  • a few script-level test cases ;)

@akroviakov
Copy link
Collaborator

@pdamme I have a few further questions:

  1. When defining matrix as a literal containing other literals (as is evident from "Matrix literals" name), a antlr4::tree::TerminalNode* is created in LiteralContext, which allows an access to the string (e.g., mat[1,2,3]), which can then be parsed into elements through regex for any further processing. I didn't find a way to get the composite literals, is there any?
    1.1. What are the reasons for matrix creation not to be an expression of literals, am I missing something important? This way we get std::vector<LiteralContext *>, which is a bit more convenient to work with. Besides, any literal is an expression itself.
  2. I tried implementing the expression of literals variant for uint64_t only, since it was a bit easier to get started.
    2.2 I create a std::shared_ptr<uint64_t[]>, which will be filled with literals translated to mlir::Value and then to primitive type as done in DaphneInferShapeOpInterface. This std::shared_ptr<uint64_t[]> is then used as an argument in DenseMatrix constructor.
    2.3. CmakeLists in src/daphne/dsl had to be modified to link DataStructures.
    2.4. The return of visitMatrixExpr is:
            return  builder.create<mlir::daphne::MatrixConstantOp>(loc, utils.matrixOf(builder.getIntegerType(64, false)),
                static_cast<mlir::Value>(
                    builder.create<mlir::daphne::ConstantOp>(
                        loc,
                        builder.getIntegerAttr(builder.getIntegerType(64, false), reinterpret_cast<uint64_t>(mat)))
                )
    ));
  1. The issue I'm facing when running a test script looks like this:
JIT session error: Symbols not found: [ _print__DenseMatrix_uint64_t__bool__bool ]
JIT-Engine invocation failed: Failed to materialize symbols: { (main, { _mlir__mlir_ciface_main, _mlir_ciface_main, _mlir_main, main }) }Program aborted due to an unhandled Error:
Failed to materialize symbols: { (main, { _mlir__mlir_ciface_main, _mlir_ciface_main, _mlir_main, main }) }
Aborted (core dumped)

And I'm not sure of its exact origin. What could cause this, was it because I chose to go the "expression of literals" way?

@pdamme
Copy link
Collaborator

pdamme commented May 29, 2022

Hi @akroviakov,

  1. You shouldn't need to do any parsing via regex youself, just use ANTLR for all the parsing. You can access the nested literals parsed by ANTLR via something like ctx->literal(i) (have a look at visitCallExpr, I think that's structurally quite similar).
    1.1. What I meant was that everything inside the matrix literal shall be known at parse-time, for now. So the user may write [1, 2, 3], but not [a + log(b), sum(c), 2^3], and not even [1+2, 2, 3]. So the DaphneDSL grammar should not even allow general expression for the elements of a matrix literal, for now. We can and should relax this later, as a separate issue. I'm not sure what exatly you mean by expression of literals. I was thinking of an ANTLR parser rule like (take care that it doesn't get confused with right indexing):

    expr:
        literal # literalExpr
        '[' ( literal (',' literal)* )? ']' # matrixLiteralExpr
        ...

    You could also outsource it as a separate parser rule like:

    expr:
        literal # literalExpr
        matrixLiteral # matrixLiteralExpr
     ...
    
    matrixLiteral:
        '[' ( literal (',' literal)* )? ']' ;
  2. I recommend using int64_t, since that's the default integer type in DaphneDSL. That should also solve the problem in point 3 for now. But anyway, eventually all DaphneDSL value types should be supported in matrix literals.
    2.2. Sounds good.
    2.3 Totally fine and expected. (I guess you mean src/parser/daphnedsl.)
    2.4 Looks good to me, assuming that mat is of type DenseMatrix*. I think you can omit the static_cast in line 2.

  3. This error (JIT session error: Symbols not found:) typically means that some kernel was not pre-compiled for the required combination of input and output types. The error message also indicates which kernel. Here it's print (printObj) for DenseMatrix of uint64_t. You can simply add the necessary instantiation in src/runtime/local/kernels.json (search for printObj).

Hope that helps.

@akroviakov
Copy link
Collaborator

akroviakov commented May 30, 2022

@pdamme Thanks for explanations. Yes, that is the parser rule I used. I was wondering whether the emphasis on matrix literal could mean it to be an explicit literal like:

MATRIX_LITERAL:
   [' ( (INT_LITERAL (',' INT_LITERAL)*)? | (FLOAT_LITERAL (',' FLOAT_LITERAL)*)? | (STRING_LITERAL (',' STRING_LITERAL)*)? ) ']';

And thanks for your explanation regarding JIT session error: Symbols not found:, simply changing type to int64_t made everything work ;).

And the final question: since our rule defines matrix content as ( literal (',' literal)* )? , we leave a possibility of an empty matrix. This, however, makes it impossible to infer the matrix type. Should the rule be changed or should the implementation throw an error for now?

pdamme pushed a commit that referenced this issue Jun 6, 2022
* DaphneDSL matrix literal

- Creates a `DenseMatrix` with shape (nx1) of type `int64_t`,  `double` or `bool` inside `DaphneDSLVisitor`.
- Usage example: `x = [1, 2, 3]` or `x = [1.1, 2.2, 3.3]` or `x = [true, false, true]`.
- Throws error in case of:
- - You try to create a matrix with mixed types (e.g., `x = [1, 2.2, 3]`).
- - You try to create an empty matrix: can't infer the type.
- - You use a not yet defined in `ValueTypeUtils` type (e.g., `const char*`).
- A light `bool` type support for `DenseMatrix` is added (for print and reshape kernels).
- Closes #358.
@pdamme
Copy link
Collaborator

pdamme commented Jun 6, 2022

Thanks for your PR, @akroviakov. I've just merged it in and left a comment on your final question there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants