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

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.



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


================
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)
----------------
fhahn wrote:
> 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?
Yes, this looks great, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76793