[PATCH] D102478: [Matrix] Emit assumption that matrix indices are valid.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Just a couple of nits here, basically see how much we can put in the 'cheap to check' branch.



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1944
+    llvm::Value *Idx = LV.getMatrixIdx();
+    const auto MatTy = LV.getType()->getAs<ConstantMatrixType>();
+    llvm::MatrixBuilder<CGBuilderTy> MB(Builder);
----------------
I think the linter here is right, we require const auto * here.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1946
+    llvm::MatrixBuilder<CGBuilderTy> MB(Builder);
+    if (CGM.getCodeGenOpts().OptimizationLevel > 0)
+      MB.CreateIndexAssumption(Idx, MatTy->getNumElementsFlattened());
----------------
Is it worth putting the MatrixBuilder line inside of this branch too?  Perhaps all the added code?


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:2092
+      llvm::MatrixBuilder<CGBuilderTy> MB(Builder);
+      if (CGM.getCodeGenOpts().OptimizationLevel > 0)
+        MB.CreateIndexAssumption(Idx, MatTy->getNumElementsFlattened());
----------------
Same here on both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102478