[PATCH] D66688: [LoopVectorize] Leverage speculation safety to avoid masked.loads

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:977
+static bool mustSuppressSpeculation(const LoadInst &LI) {
+  if (!LI.isUnordered())
+    return true;
----------------
reames wrote:
> MaskRay wrote:
> > reames wrote:
> > > MaskRay wrote:
> > > > Is this isUnordered or isSimple?
> > > > 
> > > > IIUC isSimple is a subset of isUnordered.
> > > Well, it's written as intended, but that might not be what you're trying to ask?  Do you have an example you're wondering about?
> > In D87538, `VectorCombine::vectorizeLoadInsert` uses the utility to suppress some cases. It uses `Load->isSimple()` but I don't know whether it should be isUnordered or mustSuppressSpeculation should use isSimple. Appreciate if you can take a look at that piece of code (you can ignore the most complexity there. The main thing is that it is a load widening)
> It is safe to speculate an unordered atomic load.
> 
> We're conservative in a bunch of places in the optimizer by using isSimple where isUnordered is legal, please don't add new ones if you can avoid it.  
In general, it is not safe to combine arbitrary atomic loads as we may not be able to lower a wider atomic.  The code in InstCombine you reference also looks correct, but I don't really see the connection between the two reviews.  They're reasoning about different aspects of legality.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66688