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

[Warren Ristow via Phabricator via llvm-commits]

wristow marked 3 inline comments as done.
wristow added a comment.

Thanks @fhahn.  I'll update the patch to address those comments.



================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:227
+
+  bool isLegalNTLoad(Type *DataType, unsigned Alignment) { return true; }
+
----------------
fhahn wrote:
> 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.
Sounds good.  I suspect that change will require making some test-changes to be compatible with the more conservative default.  Assuming it does, I'll update the patch here for any further comments, rather than directly committing.  FTR, I'm going to use my intermediate suggestion of checking the whether `DataType` is minimally aligned at `Alignment` (rather than returning `false`), under the assumption that many architectures //do //support aligned non-temporal references, and if these new utilities were ever used more generically (outside of vectorization checks), then it would be more meaningful to return `true` in those aligned cases.


================
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.
----------------
fhahn wrote:
> Can we drop the } else here, as we return in the if () case?
Good point.  Will do.


================
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");
----------------
fhahn wrote:
> 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?
That seems like a good idea, but I'm not sure how we'll exercise that code with current architectures we support.  That is, I think none of our architectures have support for any misaligned non-temporal vector memory references.  So in practice, vectorization in these cases will always be suppressed (and so the arbitrary selection of 2 vector elements would behave identically if, for example, we arbitrarily picked 4 or 8).


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

https://reviews.llvm.org/D61764