Page MenuHomePhabricator

[Matrix] Implement + and - operators for MatrixType.
ClosedPublic

Authored by fhahn on Mar 25 2020, 11:34 AM.

Details

Summary

This patch implements the + and - binary operators for values of
MatrixType. It adds support for matrix +/- matrix, scalar +/- matrix and
matrix +/- scalar.

For the matrix, matrix case, the types must initially be structurally
equivalent. For the scalar,matrix variants, the element type of the
matrix must match the scalar type.

Diff Detail

Event Timeline

fhahn created this revision.Mar 25 2020, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 11:34 AM
fhahn updated this revision to Diff 254598.Apr 2 2020, 12:35 PM

Implement conversions for matrix/scalar variants.

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 3:26 AM
fhahn updated this revision to Diff 262901.Fri, May 8, 10:52 AM

Rebase on top of the final version of D72281.

I think this patch is now ready for review.

fhahn added a comment.Mon, May 18, 3:12 AM

ping :)

this patch should be ready to be reviewed independently (it's marked as dependent on D76791 in Phab, but it can be applied independently and changing the relationship in Phab will trigger a bunch of unnecessary emails)

rjmccall added inline comments.Fri, May 22, 7:25 PM
clang/lib/Sema/SemaExpr.cpp
11962

I would suggest checking some preconditions and then just calling PrepareScalarCast.

You should allow implicit conversions from class types, which somewhat surprisingly I'm not sure we have a convenient method for, but which you can find workable code for in ConvertForConditional in SemaExprCXX.cpp. Test case is struct DoubleWrapper { operator double(); };, and you should test using that even when the element type isn't a double.

In SemaOverload, you should add builtin candidates for these operators if one operand or the other is a matrix type. Basically:

  1. Collect matrix types in AddTypesConvertedFrom
  2. For each matrix type M on the LHS, add candidates for (M, M) -> M and (M, E) -> M, and then analogously on the RHS. Although you might need to avoid adding redundant candidates if the same type shows up on both sides.
fhahn updated this revision to Diff 266303.Tue, May 26, 12:49 PM
fhahn marked an inline comment as done.

Add support for user-defined conversion function, use PrepareScalarCast and add overloads for matrix +/- operators.

fhahn added inline comments.Tue, May 26, 12:50 PM
clang/lib/Sema/SemaExpr.cpp
11962

Thank you very much John, I hope I managed to update the patch properly according to your comments!

rjmccall added inline comments.Wed, May 27, 1:29 PM
clang/lib/Sema/SemaOverload.cpp
8584

Idiomatically, we just copy QualType instead of binding references to it.

8596

I don't think this works if one side or the other has e.g. a templated conversion to a matrix type. You need to add:

(M1, M1) -> M1
(M1, M1.ElementType) -> M1
(M2, M2) -> M2    // but don't introduce the same candidate twice if the same type is also in the LHS set)
(M2.ElementType, M2) -> M2

Test case is something like:

struct identmatrix_t {
  template <class T, unsigned N>
  operator matrix_type<T, N, N>() const {
    matrix_type<T, N, N> result = {};
    for (unsigned i = 0; i != N; ++i) result[i][i] = 1;
    return result;
  }
};
constexpr identmatrix_t identmatrix = identmatrix();

...
m + identmatrix

Also, these operator candidates are only for specific operators, right? I think you can check what the operator is.

fhahn marked 2 inline comments as done.Wed, May 27, 2:24 PM
fhahn added inline comments.
clang/lib/Sema/SemaOverload.cpp
8596

Oh I see, I've updated the code to add the versions as suggested.

Also, these operator candidates are only for specific operators, right? I think you can check what the operator is.

Yes they are. I had to update addGenericBinaryArithmeticOverloads to pass the Op kind.

fhahn updated this revision to Diff 266675.Wed, May 27, 2:24 PM

Fixed overload candidates, use copy instead of const reference, thanks!

rjmccall added inline comments.Wed, May 27, 3:47 PM
clang/lib/Sema/SemaExpr.cpp
11962

Does it work to just do the initialization step instead of separately checking for an arithmetic type? That code is definitely capable of doing arithmetic conversions.

clang/lib/Sema/SemaOverload.cpp
9179–9180

Maybe you should just extract your new code into its own method and call it here. That might fit in a bit cleaner to the design, since unlike the vector case you're not planning on uniformly having element-wise versions of all of the operators.

fhahn updated this revision to Diff 266842.Thu, May 28, 6:24 AM
fhahn marked 4 inline comments as done.

Use initialization step for all conversions (including for arithemtic types), add & call separate addMatrixBinaryArithmeticOverloads

fhahn added inline comments.Thu, May 28, 10:20 AM
clang/lib/Sema/SemaExpr.cpp
11962

Yes it does. Much simpler, thanks!

clang/lib/Sema/SemaOverload.cpp
9179–9180

Sounds good, thanks!

Thanks, functionality is looking good for the non-assignment operators.

clang/lib/Sema/SemaExpr.cpp
11998

You need to not actually apply this conversion to the LHS if this is a compound assignment. You can handle that in a follow-up patch, I think, since this patch isn't doing those yet.

clang/lib/Sema/SemaOverload.cpp
8586

"operator overloads"

clang/test/CodeGenCXX/matrix-type-operators.cpp
374

Please don't test for exact value numbers; it makes updating the test really painful for relatively minor IR changes. Use FileCheck variables instead.

More generally, can you make these tests a little more targeted instead of testing the entire function body? You might find it easier to do so if you do a single operation per function.

clang/test/SemaCXX/matrix-type-operators.cpp
188

expected-note can take a repeat count

fhahn updated this revision to Diff 267061.Thu, May 28, 3:27 PM
fhahn marked 7 inline comments as done.

Updated tests to use more targeted checks, fix typo and rebased on top of master (so this change can be applied before D76791.

clang/lib/Sema/SemaExpr.cpp
11998

I am planning on adding support for compound assignment as follow-on patch and include the change there.

clang/test/CodeGenCXX/matrix-type-operators.cpp
374

I've split up the tests more and updated the check lines to only check the bits relevant for the operator (loading operands, computation, storing result). The general matrix-type tests should cover checking loading the correct value. Was that what you had in mind?

clang/test/SemaCXX/matrix-type-operators.cpp
188

Ah that's great, thanks!

rjmccall accepted this revision.Fri, May 29, 12:20 PM

LGTM.

clang/lib/Sema/SemaExpr.cpp
11998

WFM

clang/test/CodeGenCXX/matrix-type-operators.cpp
374

Yes, this looks great, thanks!

This revision is now accepted and ready to land.Fri, May 29, 12:20 PM
This revision was automatically updated to reflect the committed changes.