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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:235
     /// have that type, with a cast if necessary.
-    Value *expandCodeFor(const SCEV *SH, Type *Ty = nullptr);
-
+    Value *expandCodeFor(const SCEV *SH, Type *Ty = nullptr, bool Root = true);
 
----------------
mkazantsev wrote:
> My concern here is that now, as it seems, it is only valid to call `expandCodeFor` with `Root = false` when expanding sub-expression for another expression. But because by default Root is true, people who are not aware of this might easily make mistakes in the future (it is intuitive to just call expand and not pass root param unless I have to).
> 
> I'd suggest the following:
> - Have private version of `expandCodeFor` without default value for this param, so that we must always specify it explicitly;
> - Have public version of `expandCodeFor` that calls `expandCodeFor( .., Root = true)`.
> 
> In that way, it will be harder to make this kind of mistakes. WDYT?
> In that way, it will be harder to make this kind of mistakes. WDYT?

yes that makes a lot of sense, thanks! I dropped the `Root` argument from `expandCodeFor` and added a `expandCodeForImpl` that takes `Root` without default. `expandCodeForImpl` is supposed to be used internally by SCEVExpander, while `expandCodeFor` is for external clients and passes `Root = true`.


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71538