[PATCH] D100663: [LV] Add undef incoming value to loop-exit phis for the middle-block.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 338374.
fhahn added a comment.



In D100663#2695304 <https://reviews.llvm.org/D100663#2695304>, @reames wrote:

> This is incorrect and will result in subtle miscompiles.
>
> Why?  Because SCEV is queried over the IR before point of undef insertion and replacement.  SCEV will - correctly - assume that it can substitute any valid concrete value for the undef.  However, the actual value later inserted is more constrained than undef.  As such, the intermediate SCEV results - both as used immediately and as cached - will be incorrect for the final IR.

Thanks for taking a look and apologies for not doing a good job at conveying my thinking. My rational was that at the point we add the undef incoming value, it is yet undetermined which edge will be taken, so SCEV will constrain the undef value, but only to the incoming value of the scalar loop. In the final IR, both incoming values should result in the same value at runtime. Does that make sense or am I still missing something?

After writing this up, I went back and checked what we use as temporary condition and realized that we are using `true,` which means that the correct incoming value from the scalar loop is dead, until we update the branch condition. This means SCEV would indeed be able to assume an arbitrary value for the PHI, which indeed would be incorrect. But I think we may be able to avoid this problem, by making the condition false initially, to make the undef incoming value dead, until we set the final branch condition. In this patch, I moved the code for setting the condition to happen after we are done with vector-code generation. The main uses of SCEV (runtime check generation) should happen before the `completeLoopSkeleton`, so it may be OK to keep the code there as well, but it seems safer to wait until the phis are actually fixed.

Note that this patch also splits up the code to create and set the condition, so there are no changes in the generated IR (creating the condition at the point it is set would result in a few instructions getting reordered). I am more than happy to discuss the best place, if the direction looks good in general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100663

Files:
  llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
  llvm/test/Transforms/LoopVectorize/scev-verify-ir.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D100663.338374.patch
Type: text/x-patch
Size: 11152 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210418/1a09a64f/attachment.bin>