[PATCH] D71538: [SCEVExpander] Preserve LCSSA directly.

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev accepted this revision.
mkazantsev added a comment.
This revision is now accepted and ready to land.

Let's give it a try. :) LGTM with some nits inline.



================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:349
+    /// Insert code to directly compute the specified SCEV expression into the
+    /// program.  The inserted code is inserted into the SCEVExpander's current
+    /// insertion point. If a type is specified, the result will be expanded to
----------------
`The inserted code is inserted` -> `The code is inserted`; too many insertions for one sentence :)


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1791
+    if (auto *Inst = dyn_cast<Instruction>(V)) {
+      // Create a temporary instruction to at the current insertion point, so we
+      // can hand it off to the helper to create LCSSA PHIs if required for the
----------------
fhahn wrote:
> mkazantsev wrote:
> > I don't quite get this logic. If all you need it for is an insertion point, why not just take next/prev iterator if `Inst` in its block?
> This code here is a preparation step so we can use `formLCSSAForInstructions` to fix the LCSSA form. `formLCSSAForInstructions` will insert LCSSA phi nodes as required for all uses of a given instruction. 
> 
> The problem here is that we do not have a use at the insertion point yet, as the caller will insert the use. So to conveniently use the existing  `formLCSSAForInstructions`, it is easiest to just create a temporary user at the insertion point, so LCSSA phis are inserted for uses at insertion point, if required. Does that make sense?
I get it, thanks. Looks fishy, I'm certain there should be a cleaner way to do it, but I cannot come up with one on the top of my head. Please add a TODO to consider a better way to do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71538