This is an archive of the discontinued LLVM Phabricator instance.

[LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, and loop induction variable.
ClosedPublic

Authored by Whitney on Apr 11 2019, 7:10 AM.

Details

Summary

This PR extends the loop object with more utilities to get loop bounds, step, and loop induction variable. There already exists passes which try to obtain the loop induction variable in their own pass, e.g. loop interchange. It would be useful to have a common area to get these information.

/// Example:
/// for (int i = lb; i < ub; i+=step)
///   <loop body>
/// --- pseudo LLVMIR ---
/// beforeloop:
///   guardcmp = (lb < ub)
///   if (guardcmp) goto preheader; else goto afterloop
/// preheader:
/// loop:
///   i1 = phi[{lb, preheader}, {i2, latch}]
///   <loop body>
///   i2 = i1 + step
/// latch:
///   cmp = (i2 < ub)
///   if (cmp) goto loop
/// exit:
/// afterloop:
///
/// getBounds
///   getInitialIVValue      --> lb
///   getStepInst            --> i2 = i1 + step
///   getStepValue           --> step
///   getFinalIVValue        --> ub
///   getCanonicalPredicate  --> '<'
///   getDirection           --> Increasing
/// getInductionVariable          --> i1
/// getAuxiliaryInductionVariable --> {i1}
/// isCanonical                   --> false

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jdoerfert added inline comments.Apr 29 2019, 8:58 PM
llvm/include/llvm/Analysis/LoopInfo.h
581

Maybe mention how one is supposed to determine this precondition.

603

Do you check, or enforce, that the Step is "canonical", e.g., the step is on the right?
I tried to find this below but I couldn't.

606

Do you check, or enforce, that the ICmpInst is "canonical", e.g., the bound is on the right?
I tried to find this below but I couldn't.

610

Doesn't this contradict the getStepInst().getOperand(1); in line 599?

637

I'd start comments with an upper case letter, again below. (sorry for the nitpicking).

644

Please rename if it is "the latch condition" to sth like "LatchCmpInst".

650

else "None".

658

Do you really test that? The initial loop fusion patch did not.

677

"in a" or "in the", but I might be wrong here.

llvm/lib/Analysis/IVDescriptors.cpp
1057

Does this work if there is no loop latch? I have the feeling Phi->getIncomingValueForBlock might choke on a nullptr. I might be wrong though.

I feel the changes in this file are unrelated and can go before the rest. If you agree, open a new patch for them, maybe with a test, and I'll accept it as it is pretty straight forward.

llvm/lib/Analysis/LoopInfo.cpp
194

[I don't care too much but I'm interested:] Any reason you separated setIniitalIVValue and setStepInst?

220

I am not sure this is in sync with the constant getOperand(1) in one of the headers.

224

Why do you have the non-SE path above for certain special cases e.g.,. non-equality conditions, before you have a, what seems to be, general SE based handling?

257

Couldn't we return nullptr here as well? It seems nullptr is a valid return value here anyway.

267

[IMPORTANT] How do we know or enforce this?

288

-"where"
+"if"

290

"and a definite successor of a loop exit block (trivially) post-dominates the other successor.
The second condition ensures the branch is a control dependence of the loop."

306

Maybe we need a more general picture here as the example shows the simple case. I can imagine some "ascii-art" or a more elaborate description.

318

if (!GuardBB) return nullptr

319

GuardBI is overwritten before used.

336

The comment is outdated (ExitBlock)

340

We often use the pattern:

if (!Visited.insert(GuardBB).second) 
  return nullptr;
356

Why do we "compute" the index of the Successor if we assert it is 1 anyway? This seems like a specialization of a more general scheme.

359

I'm always for newlines to mark control condition changes

360

This is the general version (see above) again.

369

You could instead call Visited.clear() here but I don't mind a new object.

Whitney marked 14 inline comments as done.Apr 30 2019, 1:33 AM
Whitney added inline comments.
llvm/include/llvm/Analysis/IVDescriptors.h
319

I added the check IK == IK_FpInduction which is required in the comment the induction type is FP. Originally we don't need to check that, because InductionBinOp is not set for non FP inductions, my change below will set it for cases that are not FP.

327

same as above

llvm/include/llvm/Analysis/LoopInfo.h
581

As LoopBounds take a reference of IndVar, meaning that LoopBounds can only be created if an induction variable. Is it better if I remove the comment?

606

It is enforced by getCmpInst().

610

in line 599, we are looking at the instruction which update the induction variable, and getting the step. While here we are talking about the latch compare instruction, and checking if operand(1) is the step instruction.

658

I may not have understand the question correctly. getGuard() checks if the condition satisfy the latch condition by comparing the operands and the predicate of the branch instruction with the bounds of the loop.

llvm/lib/Analysis/IVDescriptors.cpp
1057

I was trying to follow the similar semantic as the line above. However, I agree that it is a good idea to check if the latch is nullptr first.

I will open a new patch with the changes on IVDescriptors.

llvm/lib/Analysis/LoopInfo.cpp
194

The reason is there are getInitialIVValue() and getStepInst(). I don't have strong feeling either ways.

220

I think you are talking about the getOperand(1) in getStepInst(), which is looking at the StepInst, and here we are looking at the LatchCmpInst.

224

when the predicate is not ne and not eq, then there is no need to use SE, we can simply flip the strictness of the predicate.

356

There is not just asserting the index of the successor is 1, it is asserting either the successor which skip the loop is 1 or Succ is index 1 (i.e. the successor which skip the loop is 0)

360

see above reply.

369

I prefer a new object, because we don't need to keep track if the live range of the other Visited. If you don't have a strong feeling, I will keep it as is.

kbarton requested changes to this revision.Apr 30 2019, 8:33 AM
kbarton added inline comments.
llvm/include/llvm/Analysis/LoopInfo.h
595

"which update" -> "that updates"

597

instrucion -> instruction

601

"get updated for" -> "gets updated by in"

603

This is assuming the step instruction has a very specific form. Is it safe to assume this all the time (it may be and I just haven't got to that part of the code yet).

606

Same - this is assuming a very specific format for the compare. Is it guaranteed to be true in all cases?

610

Can you clarify this comment?
Does this method assume:

  1. The first successor of the latch branch is the loop header
  2. The LHS of the latch comparison is the StepInst?

and if both of these conditions are not met, then what does it return?

624

Can IndVar be made const?

627

"which update" -> "that updates"

628

Is it strictly necessary that the IV be the LHS?
What if you write:

(for i=0; N>i; i++)

Does LLVM switch it back to always be on the LHS?

632

Can you clarify this comment?
If the bounds are not constant, it should return Unknown right?

638

This is minor, but I think a more descriptive variable name would be useful here. Something like InitialIVValue, or even InitialValue.

656

I would have expected the guard to dominate the preheader, and not have another block between the guard and the preheader. Or maybe I misunderstand this point.

657

"a" loop exit block, or "the" loop exit block?
Do these methods work on loops that have multiple exit blocks?
If they do work on loops with multiple exit blocks, then I would think that all loop exit blocks need to dominate the other successor.

659

What do you mean the loop initial value?

I think what you're trying to say is guard condition guarantees the loop body should be executed at least once. The only way I can think to say that using the current terminology is the lower bound of the loop is less than the upper bound (although that may not cover all possible conditions).

668

I find this very confusing.
I would have expected the getInductionVariable to return a Value, not an instruction. Strictly speaking, I don't think a PHINode is considered a variable.

What is the utility of returning the PHINode here instead of the actual Value?

669

I'm not a big fan of boolean parameters. They're generally an indication that you're doing too much in the method and it should be split up into two separate methods.

672

varaible -> variable

680

This seems to be more restrictive then the check for induction variable.
Specifically, not being used outside the loop and the step instruction being an add or sub.

682

The auxiliary induction variable is not required to be used or cannot be used?
What happens if it is used in the conditional branch? Can that happen?

llvm/lib/Analysis/LoopInfo.cpp
226

How do you know to use the signed version, instead of the unsigned version?

226

Do you handle the ULE/SLE case anywhere?

289

I'm confused why one successor of the guard should dominate the preheader, not be the preheader itself.

331

This will miss the case where a basic block has multiple predecessors, but all of the predecessors are to the same block.
I don't think this is a big deal, just pointing it out. I think if you've run simplifycfg prior to this, that should get cleaned up.

370

Is it possible for getUniqueExitBlocks to add null pointers into the ExitBlocks?
I would have thought this is not possible (i.e., something is wrong if that's happening). Maybe it would be better to assert(ExitBlock && "Expecting valid ExitBlock") before the while loop to catch this if it happens.

375

This is just checking that one of the exit blocks goes to the "other" successor of the guard.
Do you not need to check that all of the exit blocks go to that block?

453

How do you know the induction variable is always operand 0 of CmpInst?

468

Can you use a phi iterator here instead?
for (PHINode &PN : Header->phis()) { ... }

471

Doesn't this assert mean the only phis you expect to find in the header are from the latch block?
Is that always going to be true?

523

fpr -> for ?
If not, what does fpr stand for?

llvm/unittests/Analysis/LoopInfoTest.cpp
33

Did clang-format format this for you?
The formatting is different then the formatting above.

This revision now requires changes to proceed.Apr 30 2019, 8:33 AM
jdoerfert added inline comments.Apr 30 2019, 10:54 AM
llvm/include/llvm/Analysis/IVDescriptors.h
319

Is this unrelated to the main part as the changes wrt setting the InductionBinOp below?

llvm/include/llvm/Analysis/LoopInfo.h
581

No, keep it.

However, later, e.g., in setStepInst, you assert(isInductionPHI(IndVar)) or something similar. The comment states this assertion in words but it could mention the isInductionPHI function which allows to verify the precondition outside of the LoopBounds.

606

getCmpInst() looks for an instruction on the LHS, that does not mean anything on its own.

610

OK.

llvm/lib/Analysis/IVDescriptors.cpp
1057

I was trying to follow the similar semantic as the line above. However, I agree that it is a good idea to check if the latch is nullptr first.

You are right, it should work, no need to check for nullptr but you can.

I will open a new patch with the changes on IVDescriptors.

Thanks.

llvm/lib/Analysis/LoopInfo.cpp
194

If that is the only reason I'd merge them into an initialize method or sth.

220

Probably/Hopefully, correct.

356

You are obviously right here, ...

I should not do code reviews at night...

369

No strong feeling, keep it.

375

That is a good point, it should for sure be stated clearly what is computed here.

Whitney marked 26 inline comments as done.Apr 30 2019, 12:17 PM
Whitney added inline comments.
llvm/include/llvm/Analysis/IVDescriptors.h
319

Created https://reviews.llvm.org/D61329 for changes in IVDescriptors.

llvm/include/llvm/Analysis/LoopInfo.h
610

It returns the predicate when the two points are satisfied.
e.g. if the second successor of the latch latch is the loop header instead of the first, then it returns the inverse predicate.
I will modify the comment to clarify that.

656

It is possible to have blocks in between the guard and the preheader, the simplest example would be empty blocks. one successor dominates the loop preheader ensures that the guard dominate the preheader.

657

It should work on loops that have multiple exit blocks as well. Why do all loop exit blocks need to dominate the other successor? The guard should just skip the loop, and all loop exit blocks are outside of the loop, as long as one of them dominate the other successor, then it proves the guard skip the entire loop (given that one successor dominate the preheader which is the only entry point to the loop). Please correct me if I am wrong.

659

The reason I use the term loop initial value instead of lower bound is the loop maybe decreasing.

668

An existing function getCanonicalInductionVariable() also return PHINode *. Which value do you mean by the actual Value?

680

Right, that's the intensional behaviour

682

It can be used, it is not *required* to be used.

llvm/lib/Analysis/LoopInfo.cpp
226

getCanonicalPredicate() is used by getGuard() which get the Signed Predicate for both the latch and the potential guard to compare, so it doesn't matter if SLE or ULE is returned.

289

because it may not be the case, simplest example would be empty blocks

331

hmmm....in that case, maybe I should use getUniquePredecessor() instead of getSinglePredecessor().

370

ExitBlock is overriden in the loop ExitBlock = ExitBlock->getSingleSuccessor();, so ExitBlock can be nullptr.

375

The purpose of the guard should just skip the entire loop, given that one successor dominates the loop preheader, which is the only entry point to the loop, then any loop exit block dominates other successor should be enough to prove that it skips the loop, as loop exit blocks are not part of the loop. Please correct me if I am wrong.

471

this assert means any phis in the header should have an incoming value from the latch block. This should always be true, because by definition the exists a branch from the latch to the header if latch exists.

llvm/unittests/Analysis/LoopInfoTest.cpp
33

That's from clang-format, I guess is because of the longer function name.

Generally, I'd prefer to use ScalarEvolution over inspecting (and expecting specific) instructions in a loop, whenever possible. Some canonicalization passes are designed for this. In particular, IndVarSimplify used to make canonical loops (i.e. start at zero, increment by one). r133502 introduced -disable-iv-rewrite to rely more on ScalarEvolution instead of "opcode/pattern matching" (cite from the commit message). -enable-iv-rewrite=false was made the default in r139579 after finding that it slows down many benchmarks. It was completely removed in r153260.

That is, such pattern matching will miss many loops that could have detected with ScalarEvolution or induction-variable rewriting. We could either re-introduce iv-rewrite or make the pattern matching more general (but also more complex) when needed. IMHO, both are less preferable.

ScalarEvolution might not cover all use cases and the LoopInterchange implementation currently is based on finding the induction variable. However, IMHO we should reduced direct uses of the induction variable instead of promoting it to an API.

Whitney updated this revision to Diff 197547.May 1 2019, 7:34 AM
Whitney marked 37 inline comments as done.May 1 2019, 7:34 AM

Addressed all the easier review comments first.

Whitney added inline comments.May 1 2019, 7:35 AM
llvm/include/llvm/Analysis/LoopInfo.h
624

No, because isInductionPHI() takes a non const IndVar.

llvm/lib/Analysis/LoopInfo.cpp
290

exit block -> ....-> the other successor
exit block dominates the other successor...
why should it change to successor of loop exit block (trivially) post dominates the other successor?

Whitney updated this revision to Diff 197592.May 1 2019, 10:45 AM
Whitney marked 6 inline comments as done.
Whitney updated this revision to Diff 197644.May 1 2019, 3:52 PM
Whitney marked 3 inline comments as done.
Whitney updated this revision to Diff 197770.May 2 2019, 6:50 AM
Whitney marked an inline comment as done.
Whitney updated this revision to Diff 197841.May 2 2019, 12:14 PM
Whitney updated this revision to Diff 198903.May 9 2019, 2:13 PM
Whitney updated this revision to Diff 199171.May 12 2019, 11:39 AM
kbarton requested changes to this revision.May 13 2019, 12:55 PM
kbarton added inline comments.
llvm/include/llvm/Analysis/LoopInfo.h
416

Minor comment, but I think it's better to make the names as specific as possible. So, this could be getGuardBranch, instead of just getGuard, to distinguish between getGuardBlock for example.

422

How is this method different from getCanonicalInductionVariable?
If these are fundamentally different, then the comments need to be clear what the differences are and when one should be used over the other.

578

This is minor, but have you tried running this through doxygen to see what the output looks like?
It may come out looking much different in the doxygen pages then it does here.

610

I think this comment needs to be clarified further as I find it confusing the way it is written.
In particular, the mixing of itemization (before the example) and enumeration (after the example) . Can you rewrite it to make it more clear?

668

Yes, I saw that now. Perhaps I'm just being too pedantic on the terminology that is being used.
I would have expected a method that returns a "Variable" to be a Value, not a PHINode (an instruction). However, you can get to the Value from the PHINode, so it's probably OK.

If others have strong feelings about this on way or another, please comment.

685

Minor comment -the parenthetical should be before the period, not after.

llvm/lib/Analysis/LoopInfo.cpp
226

Can you make it explicit in the comments for getCanonicalPredicate that it will always return the signed predicate, even if the latch uses the unsigned version?

236

If neither of these conditions are true, LoopBounds will be initialized with a null StepValue. I'm assuming this is intended behaviour.

Can you please add a comment to the getStepValue method that says it will return nullptr if the StepValue cannot be determined?

251

Should this be dyn_cast_or_null instead, to account for the possibility that getTerminator could returns nullptr?

344

I don't understand the comments below, specifically what the --> is meant to indicate.
I think it should be sufficient to say the method returns a branch instruction that branches around the specified loop.
Also, the GuardBB, SkipIndex, SkipToBlock, etc., are all local variables that don't escape this method, so they are typically not documented in the function comments.

351

Can you rename this method to indicate you are returning a branch instruction (e.g., getPotentialGuardBranch).

414

In the comments you describe the guard in terms of dominance, but you don't use dominance anywhere in the function.

I would suggest trying to refactor this as follows:

Change this method to getPotentialGuardBlock, that returns the basic block that (potentially) guards the loop body.
I think you can determine the guard block using dominators and post-dominators. Specifically, the guard block should be the closest predecessor that dominates the loop preheader. And the "other" successor of the potential guard block should post-dominate all of the blocks inside the loop. I believe that should be sufficient to guarantee the block is a potential guard.

Then you can get the branch from the guard block in the getGuard method, and do the necessary analysis on the branch itself to verify it conforms with the requirements for a guard branch. Does this make sense?

419

Again, try to make it clear in the name what you are specifically getting - in this case the guard branch.

484

It's probably reasonable here to assert that Header and Latch are not nullptr.

llvm/unittests/Analysis/LoopInfoTest.cpp
752

These tests look good. I don't see any tests for the following - could you add some:

  1. Loop Bounds and associated methods you have added
  2. Unguarded loops (i.e., loops with constant upper bounds should not get a guard).
  3. Loop nests
  4. Functions with control flow that do not "guard" the loop.
This revision now requires changes to proceed.May 13 2019, 12:55 PM
jdoerfert added inline comments.May 13 2019, 6:23 PM
llvm/lib/Analysis/LoopInfo.cpp
414

And the "other" successor of the potential guard block should post-dominate all of the blocks inside the loop.

Not all blocks but just the header I think. That should allow multi-exit loops (if it matters).

I believe that should be sufficient to guarantee the block is a potential guard.

Agreed.

Whitney marked 3 inline comments as done.May 13 2019, 6:39 PM
Whitney added inline comments.
llvm/include/llvm/Analysis/LoopInfo.h
578

Tried this with doxygen, looks as expected.

llvm/lib/Analysis/LoopInfo.cpp
251

Can a basic block not have a terminator? I thought it must have one.

484

This will definitely be true, because it just checked that the loop is in isLoopSimplifyForm()..I can add asserts.

Whitney updated this revision to Diff 199539.May 14 2019, 5:39 PM
Whitney marked 24 inline comments as done.
Whitney added inline comments.
llvm/include/llvm/Analysis/LoopInfo.h
416

changed.

422

Added comment to clarify that.

610

I updated the comment again, hopefully is more clear this time.

668

an instruction is a value as well.

685

moved.

llvm/lib/Analysis/LoopInfo.cpp
226

That's not totally true, it returns the signed predicate for ne or eq which doesn't have a sign. Updated the comment to reflect that.

236

added.

344

Updated the comment.

351

done.

414

I changed the code to use the post dominator, but there is no need to use the dominator, as by finding the closest conditional branch, we know it dominates the loop.

419

done

Whitney updated this revision to Diff 199653.May 15 2019, 11:57 AM
Whitney marked 2 inline comments as done.
Whitney added inline comments.
llvm/unittests/Analysis/LoopInfoTest.cpp
752

Added more test cases

Whitney updated this revision to Diff 199908.May 16 2019, 2:35 PM
kbarton marked an inline comment as done.May 17 2019, 11:41 AM

I discussed this with @Whitney and I think at this point it makes sense to remove the getGuard and getPotentialGuard methods as there still seems to be some discussions about the right approach for that.
Aside from that, I think everything else looks pretty good, aside from some specific style-related comments.

llvm/include/llvm/Analysis/LoopInfo.h
584

a -> an

683

I think this condition needs to be reworded to:
The other successor post-dominates all nodes in the loop.

llvm/lib/Analysis/LoopInfo.cpp
251

From: http://llvm.org/doxygen/classllvm_1_1BasicBlock.html

A well formed basic block is formed of a list of non-terminating instructions followed by a single terminator instruction. Terminator instructions may not occur in the middle of basic blocks, and must terminate the blocks. The BasicBlock class allows malformed basic blocks to occur because it may be useful in the intermediate stage of constructing or modifying a program. However, the verifier will ensure that basic blocks are "well formed".

So, while I agree it is unlikely to happen, it is still possible, depending on when this is called wrt other optimizations.

290

From what I have seen, a more common way to write this method in LLVM-style would be:

if (SCEVAddRecExpr *StepAddRecExpr = dyn_cast<SCEVAddRecExpr>(SE.getSCEV(&getStepInst()))) 
  if (const SCEV *StepRecur = StepAddRecExpr->getStepRecurrence(SE)) {
    if (SE.isKnownPositive(StepRecur))
      return Direction::Increasing;
    if (SE.isKnownNegative(StepRecur))
      return Direction::Decreasing;
  }
return Direction::Unknown;

That said, I don't know if this is explicitly documented anywhere.

307

Similarly, this is typically written as:

if (PHINode *IndVar = getInductionVariable(SE))
  return LoopBounds::getBounds(*this, *IndVar, SE);
return None;
524

Same as above. This can be rewritten by initializing IndVar in the if condition and then calling isInductionPHI.

Whitney marked 17 inline comments as done.May 17 2019, 12:01 PM
Whitney updated this revision to Diff 200090.May 17 2019, 1:34 PM
Whitney marked 6 inline comments as done.
Whitney added inline comments.
llvm/lib/Analysis/LoopInfo.cpp
290
kbarton accepted this revision.May 22 2019, 1:25 PM

Aside from a few minor comments, this LGTM.

llvm/include/llvm/Analysis/LoopInfo.h
546

Need to remove guard branch from the comments, as that is not included in this patch anymore.

llvm/lib/Analysis/LoopInfo.cpp
192

This should also be dyn_cast_or_null, since Latch->getTerminator() can return nullptr in some cases.

290

Yes, that's true. But that is dealing more with large bodies of code and/or complicated conditionals where many things need to be satisfied. I think the purpose of that is it is very hard to keep all of the logic straight in your head while reading the code, so separating out common conditions or early exits makes the code easier to read. These functions, on the other hand, are very small and do not have a lot of conditions or context that needs to be remembered.

I agree this is somewhat subjective, and I don't have a strong preference either way. However, after looking at a lot of code in LLVM, I find the more compact style easier to read for small functions.

This revision is now accepted and ready to land.May 22 2019, 1:25 PM
fhahn added a comment.May 22 2019, 2:01 PM

I still think there is some duplication with InductionDescriptor that could be unified, which would also make it easier to use this new utility in places that currently use InductionDescriptor.

Some comments inline, also InductionDescriptor already stores & provides accessors for InitialIVValue, StepValue (although just the SCEV/ConstantInt - is the generic value actually what you need?) and StepInst. With my earlier comment, I was going more in the direction of using IVDescriptor, instead of duplicating the fields here and using the existing functions to detect induction PHIs.

llvm/include/llvm/Analysis/LoopInfo.h
60

Unused?

647

This is basically the same as InductionDescriptor::getConsecutiveDirection

668

I am not sure what this means exactly. Do you have checks to exclude loops with multiple exit blocks?

Also, I am not sure what you intend on using this value for. Wouldn't it be less fragile to use SCEV to reason about the number of iterations? You can also evaluate the AddRec expression of the induction PHI at the final loop iteration.

llvm/lib/Analysis/LoopInfo.cpp
314

Isn't the main part of this function the same as InductionDescriptor::isInductionPHI() ? http://llvm.org/doxygen/classllvm_1_1InductionDescriptor.html#a1974b8e8a8bae482a772ebfab1e9c794

fhahn added inline comments.May 22 2019, 2:04 PM
llvm/lib/Analysis/LoopInfo.cpp
314

at leas conceptually

Whitney updated this revision to Diff 200821.May 22 2019, 2:32 PM
Whitney marked 4 inline comments as done.
Whitney marked 4 inline comments as done.May 28 2019, 1:49 PM
Whitney added inline comments.
llvm/include/llvm/Analysis/LoopInfo.h
60

right, will remove.

647

I believe getConsecutiveDirection only work for constant step.

668

This is the focus on the loop latch as the exiting block.

llvm/lib/Analysis/LoopInfo.cpp
314

getInductionVariable() should return an induction phi, but an induction variable may not be a loop induction variable. A loop induction variable is used direct/indirectly in loop latch to determine if the loop continues.
I will look into if I can further simplify this function.

Whitney updated this revision to Diff 201890.May 29 2019, 6:19 AM
Whitney marked an inline comment as done.May 29 2019, 6:23 AM

@fhahn Thanks for your review. Do you think this is better now? Is there some areas you would like me to look into more?

Whitney retitled this revision from [LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, induction variable, and guard branch. to [LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, and loop induction variable..May 29 2019, 11:25 AM
Whitney edited the summary of this revision. (Show Details)

If there are no further comments or concerns, I will commit this patch on this Wed.

Whitney updated this revision to Diff 202979.Jun 4 2019, 10:27 AM
fhahn added a comment.Jun 4 2019, 10:30 AM

If there are no further comments or concerns, I will commit this patch on this Wed.

Sorry for holding this up, I'll try to take another look today.

fhahn added a comment.Jun 4 2019, 11:14 PM

LGTM, thanks! Let's get this in and iterate on it as we get more places to use it.

This revision was automatically updated to reflect the committed changes.