[PATCH] D150526: [X86] Add X86FixupVectorConstantsPass to re-fold AVX512 vector load folds as broadcast folds

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FixupVectorConstants.cpp:38
+  StringRef getPassName() const override {
+    return "X86 Fixup Vector Constants";
+  }
----------------
Since this is AVX512 only, maybe it should be "X86 Fixup Vector AVX512 Constants"? Likewise for the filename.


================
Comment at: llvm/lib/Target/X86/X86FixupVectorConstants.cpp:265
+  switch (Opc) {
+  // Foldable AVX512 FP Logic Ops
+  // Attempt to use smaller width broadcast-folds.
----------------
Is it only for logic ops? Why can't we do something like `add`, `sub`, etc... iff we can `rebuildSplatableConstant` with the correct `BitWidth`?


================
Comment at: llvm/lib/Target/X86/X86FixupVectorConstants.cpp:267
+  // Attempt to use smaller width broadcast-folds.
+  case X86::VANDPSZ128rm:
+  case X86::VANDPDZ128rm:
----------------
Can we convert `rm` ops that aren't in `evex` encoding? Or since is that dangerous in some way?


================
Comment at: llvm/lib/Target/X86/X86FixupVectorConstants.cpp:370
+  case X86::VPTERNLOGQZrmi:
+    return ConvertToBroadcast(0, 0, X86::VPTERNLOGQZrmbi, X86::VPTERNLOGDZrmbi,
+                              0, 0, 3);
----------------
Why no attempt for `OpBcst256/128/16/8` on any of these? If they are truly unused can we remove this to kill the deadcode?


================
Comment at: llvm/lib/Target/X86/X86FixupVectorConstants.cpp:372
+                              0, 0, 3);
+  }
+
----------------
do all the `OpBcst\d+` opcodes not always match? Seems to be the same for all of them. If so I think it would be alot simpler to try all `OpBcst` sizes < Native_BitWidth. You could have an overload if you have want to support only checking a specific width later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150526