[PATCH] D60565: [LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, induction variable, and guard branch.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:172
+  ConstantInt *C2 = dyn_cast<ConstantInt>(&V2);
+  if (C1 != nullptr && C2 != nullptr) {
+    if (C1->getSExtValue() == C2->getSExtValue())
----------------
etiotto wrote:
> Whitney wrote:
> > fhahn wrote:
> > > LLVM coding convention is not to check for nullptr explicitly, just use if (C1 && C2) (here and below)
> > Didn't know this coding convention, will remove the nullptr checks.
> Can you point out the section in https://llvm.org/docs/CodingStandards.html that mentions that? I did a quick scan and I could not find a reference. Much of the existing code in LLVM also use nullptr in if conditional expressions. 
Weird, I thought it was in there, but couldn't find it either. Maybe it's not a spelled out thing and there are at least some checks that use ==/!= nullptr . I do not have strong feelings about it.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:191
+  // Simplify(V1) == Simplify(V2)
+  if (Instruction *I1 = dyn_cast<Instruction>(&V1))
+    if (Value *V = SimplifyInstruction(I1, I1->getModule()->getDataLayout()))
----------------
etiotto wrote:
> Whitney wrote:
> > fhahn wrote:
> > > I'm not sure we should use SimplifyInstruction here. Ideally everything should be already simplified?
> > I agree that ideally everything should be already simplified, but I encounter cases which the instructions are not simplified, and I don't think we can assume the instructions are simplified or not, as getGuard() can be called in any point in time.
> It depends. Is feasible a consumer may not have simplified the expression prior to calling this member function. So, arguably, is useful to attempt to simplify it here.
Sure, my main concern with this pattern is that it is inefficient compile-time wise, as SimplifyInstruction potentially tries to recursively simplify the instruction at hand. 

It's more a general issue, where we do this at various places, instead of being more principled and try to understand why the instructions we expect to be simplified really aren't. Do you have any data how often this triggers for the passes you have using this?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565