[PATCH] D76793: [Matrix] Implement + and - operators for MatrixType.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:12112
+    return InvalidOperands(Loc, OriginalLHS, OriginalRHS);
+  }
+
----------------
rjmccall wrote:
> 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.
I am planning on adding support for compound assignment as follow-on patch and include the change there.


================
Comment at: clang/test/CodeGenCXX/matrix-type-operators.cpp:374
+  // CHECK-NEXT:    %1 = bitcast [90 x double]* %value to <90 x double>*
+  // CHECK-NEXT:    %2 = load <90 x double>, <90 x double>* %1, align 8
+  // CHECK-NEXT:    %call = call double @_ZN14DoubleWrapper1cvdEv(%struct.DoubleWrapper1* %w1)
----------------
rjmccall wrote:
> 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.
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?


================
Comment at: clang/test/SemaCXX/matrix-type-operators.cpp:188
+    // expected-note at -3 {{candidate function}}
+    // expected-note at -4 {{candidate function}}
+    return {};
----------------
rjmccall wrote:
> `expected-note` can take a repeat count
Ah that's great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76793