[PATCH] D57428: [SCEV] Guard movement of insertion point for loop-invariants (take 2)

[Warren Ristow via Phabricator via llvm-commits]

wristow created this revision.
wristow added reviewers: skatkov, mkazantsev, sanjoy, sebpop.
Herald added subscribers: javed.absar, hiraditya.

This reinstates r347934, along with a tweak to address a problem with PHI node ordering that that commit created (or exposed).  That commit was reverted at r348426, due to the PHI node issue.

Original commit message:

  r320789 suppressed moving the insertion point of SCEV expressions with
  dev/rem operations to the loop header in non-loop-invariant situations.
  This, and similar, hoisting is also unsafe in the loop-invariant case,
  since there may be a guard against a zero denominator. This is an
  adjustment to the fix of r320789 to suppress the movement even in the
  loop-invariant case.
  
  This fixes PR30806.

Here is a reduced test-case that demonstrates the PHI node issue:

  extern void foo(char *, unsigned , unsigned *);
  extern void bar(int *, long);
  extern char *processBuf(char *);
  
  extern unsigned theSize;
  
  void foo(char *buf, unsigned denominator, unsigned *flag) {
    int incr = (int) (theSize / denominator);
    int inx = 0;
    while (*flag) {
      int itmp = inx + incr;
      int i = (int) theSize;
      bar(&i, (long) itmp);
      buf = processBuf(buf);
      inx = itmp;
    }
  }

When compiling this test-case at -O2 using r347934, an instruction is placed between PHI nodes during Induction Variable Simplification, and that causes an asssertion to fail later on:

  $ clang -c -O2 tst.c
    ...
  llvm/include/llvm/IR/Instructions.h:2714: llvm::Value* llvm::PHINode::getOperand(unsigned int) const: Assertion `i_nocapture < OperandTraits<PHINode>::operands(this) && "getOperand() out of range!"' failed.
    ...
  $

The test-case "pr30806-phi-scev.ll" in this proposed patch shows the issue when running `opt` with `-indvars`:

  $ opt -S -indvars -o x.ll pr30806-phi-scev.ll
  PHI nodes not grouped at top of basic block!
    %buf.addr.07 = phi i8* [ %buf, %while.body.lr.ph ], [ %call, %while.body ]
  label %while.body
  in function foo
  LLVM ERROR: Broken function found, compilation aborted!
  $

To be clear, this PHI node problem doesn't happen with current ToT, because r347934 was reverted at r348426 (although the integer-div-by-0 problem of PR30806 //does //still happen with ToT).  The patch proposed here is the original work of r347934, with an adjustment to the insertion-point, by checking whether the insertion-point is at a PHI node.  Specifically, it adds the following two lines of code (along with an additional test-case) to the commit of r347934:

  if (isa<PHINode>(*InsertPt))
    InsertPt = &*InsertPt->getParent()->getFirstInsertionPt();

Bluntly, I feel this is safe, but I wonder whether it is "brute-force".  That is, when the `InsertPt` is a `PHINode`, it's not legal -- and calling `getFirstInsertPt()` as shown above will fix it.  But I wonder whether the underlying bug is that the `InsertPt` should be guaranteed to never be a `PHINode` at that point in the code?

To put it another way, this problem didn't manifest prior to r347934 (or from a historical perspective, the same issue appeared when a similar SCEV fix was made at r289412, but which was reverted shortly thereafter at r289453).  Suppressing the hoisting of some expressions (which is what r347934 did) seems like it should be safe.  So it feels like there is a deeper issue in that suppressing the hoisting (because `SafeToHoist` returned false) surprisingly resulted in the placement of a non-PHI-node instruction between two PHI-nodes.  I thought if I could come up with a test-case that has this PHI-node issue irrespective of whether `SafeToHoist` returned true or false (that is, even before r347934 was made), I might have better luck analyzing it and finding a deeper root cause.  But I was unable to find a test-case that triggered this when the `SafeToHoist` returned true.  From that perspective, I considered only adding the two new lines that move the `InsertPt` when `SafeToHoist` was false, but if the addition of the above two lines is a "proper solution", it seems like it's perfectly reasonable to do that irrespective of what `SafeToHoist` returns.


https://reviews.llvm.org/D57428

Files:
  llvm/lib/Analysis/ScalarEvolutionExpander.cpp
  llvm/test/Transforms/LoopVectorize/pr30806-phi-scev.ll
  llvm/test/Transforms/LoopVectorize/pr30806.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57428.184206.patch
Type: text/x-patch
Size: 9508 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190130/9ac7a889/attachment.bin>