Missing checks in isInductionPhi function

IR & Optimizations
1

I came across an assertion which was caused due to a call to isInductionPhi() on a Phi which is present in the Loop body and not the Loop header.

def void @temp() {
  br label %1

1:                                                ; preds = %5, %0
  %2 = phi i64 [ %3, %5 ], [ 0, %0 ]
  %3 = add i64 %2, 1
  br i1 false, label %5, label %4

4:                                                ; preds = %1
  br label %5

5:                                                ; preds = %4, %1
  %6 = phi i64 [ %2, %4 ], [ 0, %1 ]
  %7 = getelementptr double, ptr null, i64 %6
  %8 = load double, ptr %7, align 8
  br i1 true, label %9, label %1

9:                                                ; preds = %5
  ret void
}

Lets consider this LLVM IR snip, for the bug. If you see the Phi (%6 = phi i64 [ %2, %4 ], [ 0, %1 ]), and call the function isInductionPhi on the given function, it will crash with an assert “Invalid basic block argument!”.

bool InductionDescriptor::isInductionPHI(
{
...
Value *StartValue =
      Phi->getIncomingValueForBlock(AR->getLoop()->getLoopPreheader());
...
}

Expanding the said function, it triggered an assertion inside the getIncomingValueForBlock function.

Value *getIncomingValueForBlock(const BasicBlock *BB) const {
  int Idx = getBasicBlockIndex(BB);
  assert(Idx >= 0 && "Invalid basic block argument!");
  return getIncomingValue(Idx);
}

Now, the assertion seems correct as, the basic block passed to this function is the Loop preheader, which is not there in the value list of the given Phi (%6). So, getBasicBlockIndex() returns -1 as a result of that.

Now, here are my questions.

  1. Is it correct to say that, Phis that are induction variables cannot be present inside a Loop body and can only be in a Loop header ? (I believe it is correct to say that)
  2. If (1) is true, should isInductionPhi have such a check ? instead of asserting, if this function is called on a Phi present inside a Loop body, it should return false.
2

@mehdi_amini Hey, who would be the right person to tag to discuss about this bug ?

3

If (1) is true, should isInductionPhi have such a check ? instead of asserting, if this function is called on a Phi present inside a Loop body, it should return false.

I think all users of isInductionPhi should ensure that they only pass header phis; induction phis need to be in the header and it’s easy for callers to ensure that only header phis are passed.

4

I agree with you, but since LLVM provides an API to be used by the callers. Instead of having an assert with difficult to understand meaning, can we rather have a simple assert or just make the function return false ?

users of isInductionPhi should ensure that they only pass header phis

This is not mentioned anywhere in the docs, and is an easy mistake to be made. Just by the name of the function, it is easy to be mistaken about how the function handles such cases.

I have patch ready that adds a simple check, should I post it on github ?

5

Yes, please post it on Phabricator, as described in our documentation.
Small changes like this can directly be placed there w/o the need for a discussion here.
Pick people that modified the relevant code last as reviewers please.

6

Instructions:
https://llvm.org/docs/Phabricator.html

7

Sure, will do so!

8

A patch to clarify the documentation + an explicit assert would be great!

1 Like
9

Hey @fhahn @jdoerfert I have posted a patch on fabricator.

https://reviews.llvm.org/D149041