[PATCH] D84180: [Matrix] Add LowerMatrixIntrinsics to the NPM

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1381
+      // should run in the backend pipeline on demand.
+      if (EnableMatrix)
+        MPM.addPass(
----------------
IIUC this references the LLVM option. Does this actually work? Shouldn't that use `LangOpts.MatrixTypes`, which is set when passing `-fenable-matrix` to Clang.


================
Comment at: clang/test/CodeGen/matrix-lowering-opt-levels.c:7
+// RUN: %clang -Os -fenable-matrix -S -emit-llvm %s -o - | FileCheck  %s
+// RUN: %clang -Oz -fenable-matrix -S -emit-llvm %s -o - | FileCheck  %s
+
----------------
Might be good to add new-pm run-lines as well.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:933
+
+  EarlyFPM.addPass(LowerMatrixIntrinsicsPass());
   EarlyFPM.addPass(SimplifyCFGPass());
----------------
SjoerdMeijer wrote:
> fhahn wrote:
> > I think we should keep this under the same enable-matrix flag like in the old pass manager for now and remove the flag for both when the pass only gets run on functions that contain matrix intrinsics.
> Ah yeah, cheers, forgot about that! Will add that.
Is there reason this is run at a different position to the legacy pass manager? If not, I think we should run them at roughly the same location, to minimize differences between new and old PM. In the old pass manager, we run before the VectorizerStart callbacks. Here this would be around line 1102 I think.


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

https://reviews.llvm.org/D84180