[PATCH] D16828: [x86] convert masked store of one element to scalar store
Sanjay Patel via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 3 09:35:36 PST 2016
spatel added a comment.
In http://reviews.llvm.org/D16828#343149, @andreadb wrote:
> In http://reviews.llvm.org/D16828#343021, @RKSimon wrote:
> > Some minor comments. I wonder if this could be moved to DAGCombiner - possibly with a test for scalar store legality and TLI.isExtractVectorElementCheap()?
> I know that on AVX, VMASKMOV does not imply the non-temporal hint. However, what about targets that implement masked stores as non-temporal stores of selected bytes? For those targets, not preserving the non-temporal hint when converting the mask store into a scalar store would affect the hardware write combining logic.
I have preserved the non-temporal hint of the original masked store in the scalar store with this patch, but...
> That said, my knowledge is only limited to x86, so I cannot say that those targets exist.. What I am trying to say is that we probably have to be careful if we decide to move this transformation into the DAGCombiner as different targets may expand those nodes in a slightly different way (not sure if this makes sense..).
> Basically, I recommend leaving it for now as a target specific combine (just my opinion).
That was my thinking too. Let's make sure this is sane across x86 first. There's a lot of potential variation in how a masked op is implemented. I'll add a 'TODO' comment to remind us that this could be lifted to DAGCombiner once we have more confidence.
More information about the llvm-commits