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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Could you please add some `.ll` tests showing how this change impacts the code being generated (if it is possible)?



================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:228
     /// program.  The inserted code is inserted into the specified block.
-    Value *expandCodeFor(const SCEV *SH, Type *Ty, Instruction *I);
+    Value *expandCodeFor(const SCEV *SH, Type *Ty, Instruction *I,
+                         bool Root = true);
----------------
Please add a comment on what is `Root`.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71538