[PATCH] D88382: [VPlan] Turn VPReductionRecipe into a VPValue

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

small nit for the commit message: might be good to mention that this also turns it into a VPUser.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7959
       WidenRecipe->getParent()->insert(RedRecipe, WidenRecipe->getIterator());
       WidenRecipe->eraseFromParent();
 
----------------
dmgreen wrote:
> fhahn wrote:
> > Now that both VPWidenREcipe and VPRedcutionRecipe are VPValues, we have to update all users of WidenRecipe with RedRecipe.
> I think these were happening in the other order, if I am understanding you correctly.  The VPWidenRecipe patch is added on top of this, and hopefully fixes up what you are referring to?
Yes I think that should be fine after checking the other patch. Looks like I looked at the patches out-of-order for the final round.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7967
                "Expected to replace a VPWidenSC");
         CompareRecipe->eraseFromParent();
       }
----------------
dmgreen wrote:
> fhahn wrote:
> > We need to update the users of VPWidenRecipe's VPValue or assert that there are none.
> Hmm. This is the cmp in a min/max pattern. I am hoping that it would only have one user that we have removed above. At least, I can't seem to get it to vectorize when it has multiple uses. I'll make sure this is an assert in the VPWiden patch.
Sounds good, Could you perhaps add an assert that the users are empty to D88447?


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

https://reviews.llvm.org/D88382