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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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



================
Comment at: clang/lib/Sema/SemaExpr.cpp:12112
+    return InvalidOperands(Loc, OriginalLHS, OriginalRHS);
+  }
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:8591
 
+  /// Add binary operators overloads for each candidate matrix type M1, M2:
+  ///  * (M1, M1) -> M1
----------------
"operator overloads"


================
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)
----------------
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.


================
Comment at: clang/test/SemaCXX/matrix-type-operators.cpp:188
+    // expected-note at -3 {{candidate function}}
+    // expected-note at -4 {{candidate function}}
+    return {};
----------------
`expected-note` can take a repeat count


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76793