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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3706-3709
+    if (StripWrapFlags) {
+      cast<Instruction>(Val)->setHasNoUnsignedWrap(false);
+      cast<Instruction>(Val)->setHasNoSignedWrap(false);
+    }
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69563