[PATCH] D79787: [IndVarSimplify][LoopUtils] Avoid TOCTOU/ordering issues (PR45835)

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

In general, I'm ok with nits regarding naming. However this `FIXME` is concerning. Could you please add a XFAIL test that shows the difference between current state and when this check is turned into assert?



================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1222
+
+  const SCEV *ExitValue; // The actual SCEV.
+  Instruction *Inst;     // Where are we expanding?
----------------
Please give it either more clear name or more clear comment. The actual SCEV of what?  Besides, please avoid including word `Value` into the name of `SCEV`s in a code that has both, it creates confusion. :)


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1223
+  const SCEV *ExitValue; // The actual SCEV.
+  Instruction *Inst;     // Where are we expanding?
   bool HighCost;  // High Cost when expansion.
----------------
Nit: maybe call it insertion/expansion point?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1227
+  Value *Expansion;
+  bool IsValidRewrite;
+
----------------
Initialization?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79787