[PATCH] D69563: [LV] Strip wrap flags from vectorized reductions

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin marked an inline comment as done.
dantrushin added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3706-3709
+    if (StripWrapFlags) {
+      cast<Instruction>(Val)->setHasNoUnsignedWrap(false);
+      cast<Instruction>(Val)->setHasNoSignedWrap(false);
+    }
----------------
lebedev.ri wrote:
> dantrushin wrote:
> > lebedev.ri wrote:
> > > dantrushin wrote:
> > > > lebedev.ri wrote:
> > > > > ```
> > > > > if(auto* I = dyn_cast<Instruction>(LoopVal))
> > > > >   I->dropPoisonGeneratingFlags();
> > > > > ```
> > > > > Shouldn't this be done outside of this loop though?
> > > > No, I think I want them cleared  from every interleaved part
> > > > 
> > > 1. Still, 
> > > ```
> > > if(StripWrapFlags)
> > >   cast<Instruction>(Val)->dropPoisonGeneratingFlags();
> > > ```
> > > 2. Isn't the actual bug is in `getOrCreateVectorValue()`? why it returns such errneous instructions?
> > 1.  But these flags are valid for Val -- it is (original) scalar instruction, which still present in original scalar loop, which serves as 
> >      reminder loop after vectorization. I want to preserve flags whereever they are valid, otherwise I just would not preserve them in
> >      InnerLoopVectorizer::widenInstruction:
> > 
> > 
> > ```
> > 4202       Value *V = Builder.CreateNAryOp(I.getOpcode(), Ops);
> > 4203      
> > 4204       if (auto *VecOp = dyn_cast<Instruction>(V))
> > 4205         VecOp->copyIRFlags(&I);
> > ```
> > 
> > 2. `getOrCreateVectorValue()`  simply returns cached vector value from `VectorLoopValueMap`.
> >    And this instruction was created by `InnerLoopVectorized::widenInstruction()`, which lacks necessary context.
> >    So we basically back to the original question - if we do not want to preserve valid flags at all, we can simply not copy them in 
> >    `widenInstruction()` (VectorOp->copyIRFlags(&I, **false**) ).
> >    But if we do want to preserve flags where they're valid, we need a context and clear them only for reductions. 
> > 
> >   
> > 
> > 
> > But these flags are valid for Val -- it is (original) scalar instruction, which still present in original scalar loop, which serves as reminder loop after vectorization. I want to preserve flags whereever they are valid, otherwise I just would not preserve them in InnerLoopVectorizer::widenInstruction:
> 
> I do not understand.
> How is the code in the current diff
> ```
>     if (StripWrapFlags) {
>       cast<Instruction>(Val)->setHasNoUnsignedWrap(false);
>       cast<Instruction>(Val)->setHasNoSignedWrap(false);
>     }
> ```
> different from what i suggest:
> ```
> if(StripWrapFlags)
>   cast<Instruction>(Val)->dropPoisonGeneratingFlags();
> ```
> ?
> Both drop NSW/NUW from `Val`.
Sorry, I misunderstood your comment. I had an impression you still want me to hoist it out of loop.
I updated diff 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69563