[PATCH] D111846: [LV] Drop integer poison-generating flags from instructions that need predication

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1223
+            State.VF.isVector() &&
+            Legal->blockNeedsPredication(Instr->getParent()) &&
+            cast<Operator>(Instr)->hasPoisonGeneratingFlags())
----------------
dcaballe wrote:
> fhahn wrote:
> > Is there a way to avoid querying information from legal here? 
> > 
> > As we are interested in any operation contributing to an address computation of a masked memory operation, could we only seed the worklist with addresses of masked operations instead (both VPWidenMemoryInstructionRecipe & VPInterleaveRecipe have `hasMask`/`getMask` helpers)? Then we would not need to check legal here, right? 
> > 
> > Checking `Instrs` location here might not be sufficient in any case I think. There could be cases where `Instr` is in a block that does not need predication, but feeds a memory instruction in a block that needs predication. In those case, I think we should also drop the flags from `Instr`. In any case, it might be good to add such a test case.
> > As we are interested in any operation contributing to an address computation of a masked memory operation, could we only seed the worklist with addresses of masked operations instead (both VPWidenMemoryInstructionRecipe & VPInterleaveRecipe have hasMask/getMask helpers)? Then we would not need to check legal here, right?
> 
> I had tried using `getMask` already. Unfortunately, there are cases where the loads are predicated but the mask has been dropped in favor of executing them speculatively unmasked. See https://reviews.llvm.org/D66688 and test `load-deref-pred.ll`. We should drop the flags from these cases as well and the only way I found is using `Legal->blockNeedsPredication`. I know it's not ideal but `Legal` is still widely used during codegen.
> 
> 
> > Checking Instrs location here might not be sufficient in any case I think. There could be cases where Instr is in a block that does not need predication, but feeds a memory instruction in a block that needs predication. In those case, I think we should also drop the flags from Instr. In any case, it might be good to add such a test case.
> 
> I think that makes sense. It would a case similar to the one that exposed the bug but moving the `subi` instruction outside the condition. I'll give it a try!
Using `getMask` can't be used for the interleave recipe either. There are interleave groups that are formed out of non-predicated loads/stores. However, the resulting interleave group may still require a mask if the to-be-generated widen load reads more elements than those needed for the group. We should not drop poison flags in those cases. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111846