[PATCH] D124284: [SLP]Try partial store vectorization if supported by target.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1361
+  /// type.
+  bool isLegalOrCustomInstruction(unsigned Op, Type *Ty) const;
+
----------------
I don't think that exposing isLegalOrCustom to the midend is the right way to go - I feel it sets a bad precedent

I don't think that "Custom" means enough to base mid-end optimizations on. It can mean anything from "this can be custom lowered to a single instruction", to "this can _sometimes_ be custom lowered to a single instruction, in specific situations, otherwise it will expand", to "this has to be custom expanded into 150 instructions". The variance between them is just too large.

It also created a dependency between the mid-end and SDAG ISel lowering that isn't good to introduce - considering that there are other ISel's like Global ISel, there might be a point in the future where SDAG is entirely unused in certain backends.

>From what I can tell (correct me if I'm wrong), what you want to add for this specific patch is a way to override/ignore getMinVectorRegisterBitWidth for stores that the target can efficiently handle. But you don't just want to change getMinVectorRegisterBitWidth?  Can we add a method for doing that? `shouldOverrideMinStoreVectorRegisterBitwidth(Type *Ty)`. The default implementation can still be the same as the current BasicTTI::isLegalOrCustomInstruction method, but it allows the target to override it if desired, and doesn't expose LegalOrCustom to the midend. Which I think is better in the long run.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124284