[PATCH] D84399: [SCEVExpander] Avoid re-using existing casts if it means updating users.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:112
+  // If we skipped past MustDominate, use MustDominate as insertion point.
+  if (IP->getParent() == MustDominate->getParent() &&
+      !IP->comesBefore(MustDominate))
----------------
fhahn wrote:
> efriedma wrote:
> > I'm not sure I understand what's happening here. "I" dominates "MustDominate" because the we're inserting an instruction that uses I. And we're assuming MustDominate itself must be a legal insertion point. So the only way we could skip past MustDominate is if IsInsertedInstruction() is true for MustDominate.
> > 
> > Is it actually possible for isInsertedInstruction(MustDominate) to be true? If it is, could we check it directly?
> 
> > Is it actually possible for isInsertedInstruction(MustDominate) to be true? If it is, could we check it directly?
> 
> Unfortunately, at the moment there is a case where we might mark existing instructions as 'inserted' (
> https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp#L1247)
> 
> This check is indeed a workaround, in case such an instruction is an insert point, so checking directly for it would be more straight forward. I'll update the code, thanks!
@efriedma, does the update make sense to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84399