[PATCH] D106714: [Matrix] RAUW should only replace an instruction in ShapeMap if supportsShapeInfo

[Florian Hahn via Phabricator via llvm-commits]

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

LGTM thanks! Some suggestions with respect to comments inline. Also, please address the clang tidy issues before committing.



================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:894
+    //
+    // Because we add to ToRemove during fusion we can't guarantee that defs
+    // are before uses.  Change uses to undef but keep track of them so that
----------------
fhahn wrote:
> Is this to fix a separate issue? The changes seem unrelated to the ShapeInfo issue.
I see, this is just the extra check mentioned in the commit message! A bit unfortunate that the extra check clutters the code a bit.

I think it might be good to adjust the comment to make it a bit clearer that we effectively replace all uses with undef and only collect/maintain `UndefedInst` for verification.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:908
+    }
+    if (!UndefedInsts.empty()) {
+      dbgs() << "Undefed but present instructions:\n";
----------------
Might be worth adding a comment here that all undefed instructions also must be removed and if not then this is a hard error.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:909
+    if (!UndefedInsts.empty()) {
+      dbgs() << "Undefed but present instructions:\n";
+      for (auto I : UndefedInsts)
----------------
Might be clearer to say '... but instruction not removed'?


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

https://reviews.llvm.org/D106714