[PATCH] D61764: [LV] Suppress vectorization in some nontemporal cases

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Sorry I totally missed this review. Overal  LGTM, just a few minor comments inline.



================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:227
+
+  bool isLegalNTLoad(Type *DataType, unsigned Alignment) { return true; }
+
----------------
wristow wrote:
> RKSimon wrote:
> > I realise this is the current default but its almost certainly better to get this return false - @fhahn any comments?
> FWIW, I have a similar view.  I was thinking of rather than returning simply true (or false), returning true only if the specified `DataType` is minimally aligned at `Alignment`.  In practice for now (since this is currently only called when checking for vectorization), this will effectively mean it defaults to returning false.  But if these routines were ever used more generally, checking the alignment of `DataType` seems like a sensible default.  I'll wait a bit to see whether @fhahn has a preference.
I agree, I think we should make non-temporal support opt in.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:755
+        // supported on the target.
+        } else if (ST->getMetadata(LLVMContext::MD_nontemporal)) {
+          // Arbitrarily try a vector of 2 elements.
----------------
Can we drop the } else here, as we return in the if () case?


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:757
+          // Arbitrarily try a vector of 2 elements.
+          Type *VecTy = VectorType::get(T, /*NumElements=*/2);
+          assert(VecTy && "did not find vectorized version of stored type");
----------------
I guess we also want to extend the cost model to ensure the vectorization factor we pick supports NT loads/stores, as a follow-up?


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

https://reviews.llvm.org/D61764