Page MenuHomePhabricator

[Matrix] Add __builtin_matrix_transpose to Clang.
ClosedPublic

Authored by fhahn on Jan 15 2020, 9:15 AM.

Details

Summary

This patch add __builtin_matrix_transpose to Clang, as described in clang/docs/MatrixTypes.rst.

Diff Detail

Event Timeline

fhahn created this revision.Jan 15 2020, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 9:15 AM
Herald added a subscriber: tschuett. · View Herald Transcript

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

fhahn retitled this revision from [Matrix] Add __builtin_matrix_transpose to Clang (WIP). to [Matrix] Add __builtin_matrix_transpose to Clang..May 13 2020, 4:01 PM
fhahn edited the summary of this revision. (Show Details)
fhahn added reviewers: rjmccall, jfb, rsmith, Bigcheese.
fhahn updated this revision to Diff 263878.May 13 2020, 4:02 PM

ping. Simplify code, extend tests. This should now be ready for review.

fhahn updated this revision to Diff 268569.Jun 4 2020, 1:03 PM

Ping.

Simplified code and update tests to use more targeted check lines.

rjmccall added inline comments.Jun 6 2020, 12:50 PM
clang/lib/CodeGen/CGBuiltin.cpp
1644

Unnecessary semicolon. I think it's probably clearer just to castAs<ConstantMatrixType>() inline in the code rather than introducing this trivial wrapper for it. We generally treat getAs/castAs as idiomatically implying the result type strong enough to permit auto, just like dyn_cast, so it'll probably even be more compact than this overall.

clang/lib/Sema/SemaChecking.cpp
1920

I don't think there's a need for this explicit check, since you're going to require the argument to have matrix type.

15046

When a builtin has custom type-checking (t), you need to handle placeholders in the operands yourself, just like you would for an operator.

15055

Don't canonicalize here, and you don't need to include const anyway.

fhahn updated this revision to Diff 269032.Jun 6 2020, 3:19 PM
fhahn marked an inline comment as done.

Thanks for all the comments!

Simplified code as suggested, handle placeholder expressions and add tests for them.

fhahn marked 3 inline comments as done.Jun 6 2020, 3:20 PM
fhahn added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
1644

Sounds good, I dropped it and replaced it with getAs

clang/lib/Sema/SemaChecking.cpp
15046

I added placeholder handling and added additional tests.

15055

dropped, thanks!

rjmccall added inline comments.Jun 6 2020, 4:24 PM
clang/lib/Sema/SemaChecking.cpp
1918

I didn't notice this before, but I think a single level of switch is fine; there's probably nothing common about matrix builtins that you're going to want to handle like this.

15046

Oh, I'm sorry, I gave you poor advice: you do need to handle placeholders, but more generally than that, you need an r-value. Since you aren't doing any further conversions, you just need to call DefaultLValueConversion, which will also handle placeholders for you.

You will also need to store the checked argument back into the call, which you can only really test with an IRGen test. This is one of the few places where we do actually directly mutate the AST.

15058

Please call getAs once and then check the result above instead of calling isConstantMatrixType() (which does most of the same work as getAs).

fhahn updated this revision to Diff 269050.Jun 7 2020, 6:24 AM

Simplify code as suggested, use DefaultLValueConversion instead of CheckPlaceholderExpr.

fhahn marked 4 inline comments as done.Jun 7 2020, 6:26 AM
fhahn added inline comments.
clang/lib/Sema/SemaChecking.cpp
1918

It's not required any more, after removing the fenable_matrix check. I dropped it.

15046

Oh right, I wasn't sure about whether we need DefaultLValueConversion here or not. I tried to construct a C/C++ test case that would actually require it, but couldn't come up with a test. In any case, I updated it to use DefaultLvalueConversion instead and also adjust the argument before returning the updated call.

rjmccall accepted this revision.Jun 8 2020, 11:13 AM

Thanks, LGTM with the given testing suggestion.

clang/lib/Sema/SemaChecking.cpp
15046

IRGen is pretty lenient about missing LValueToRValue conversions because of the way it peepholes loads from complex l-value expressions. But if you have basic constexpr stuff working in Sema, I think you can test this with lambdas. A reference to a constexpr variable that's immediately loaded from is not an ODR-use, which means that within a lambda it doesn't result in a capture. So a test like this should work, and it wouldn't if you didn't generate the LValueToRValue conversion in Sema. (I think it doesn't actually test that the LValueToRValue conversions is actually added to the AST, though, just that you actually generated the AST node.)

constexpr double4x4 m = {};
[] { return __builtin_matrix_transpose(m); }
This revision is now accepted and ready to land.Jun 8 2020, 11:13 AM
fhahn marked 2 inline comments as done.Jun 8 2020, 1:01 PM
fhahn added inline comments.
clang/lib/Sema/SemaChecking.cpp
15046

Thanks for the example. Unfortunately constexpr for matrix types are not supported directly at the moment. Currently we get

matrix-type-builtins.cpp:97:26: error: constexpr variable cannot have non-literal type 'const double4x4_t' (aka 'double  __attribute__((matrix_type(4, 4)))const')
   constexpr double4x4_t m = {};
                         ^

Something like the code below works, but it requires an explicit capture

template <class T, unsigned R, unsigned C>
using matrix_type = T __attribute__((matrix_type(R, C)));

using double4x4_t = double __attribute__((matrix_type(4, 4)));
struct identmatrix_t {
  operator double4x4_t() const {
    double4x4_t result;
    for (unsigned i = 0; i != 4; ++i)
      result[i][i] = 1;
    return result;
  }
};

void test_conversion() {
   constexpr identmatrix_t m2;
 [&m2] { return __builtin_matrix_transpose((double4x4_t) m2); };
}

Given that, would it be OK to submit with the extra test commented out and a TODO to enable it once matrix types are supported as literal type in constexprs?

Yeah, that's fine.

This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jun 9 2020, 2:44 AM

Yeah, that's fine.

Thank you very much John! I filed https://bugs.llvm.org/show_bug.cgi?id=46251 to keep track of the initializer support, with a note on enabling the commented out test.