[PATCH] D35475: [PATCH] [GVN] Ensure replacement instruction dominates its uses

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

Analyzing more, i'm confused, see the last paragraph.

> This is caused by a a PRE insertion when the predecessor block hasn't yet been through Value Numbering (due to a back-edge).

I'm not surprised this is buggy, sadly.
I suspect there are a bunch of issues around this kind of thing.

> When we then iterate over this block (later in the pass), GVN chooses to delete the first instance of the instruction as it is duplicated (instead of the later one which was created before).

This sounds like an issue with findLeader/addToLeaderTable not checking local dominance if they are in the same block, based on the assumption that it will process the block in order.
addToLeaderTable always adds to the end, and so it will hit the PRE leader before any other.

If so, the fastest fix i can think of is to  number the instructions in the block starting at 0 as we process them. An instruction can only be a leader when BB = LeaderBB iff the number is < than the current instruction's number.
Assign all PRE'd instructions numbers that are MAX_INT.

If you have two MAX_INT instructions, they are in-order already (since PRE will only add them in order).

> This is because "GVN::performScalarPREInsertion" will number the instruction as it is created (so it is in the leader-list).
> 
> The attached patch moves the "%.pre" instruction up so that the replacement is valid.
> 
> I am not sure this is a valid fix, but I couldn't figure out another solution.

There are a couple other ways to fix this.
Another would be "always check local dominance using OrderedInstruction in FindLeader ". This is equivalent to the above.

> I also tried to stop PREInsertion from running if the BasicBlock had not been numbered yet (by keeping a list of numbered blocks) but that wouldn't work since GVN only does one pass over the function (so this case would never be optimised out).

I'm unclear on how this happens at all.  Maybe you are talking about Load PRE and not scalar PRE?

Otherwise, from what I see, scalar PRE doesn't run until value numbering is completely done, except in *one* case, which is for GEP's.

  while (ShouldContinue) {
    DEBUG(dbgs() << "GVN iteration: " << Iteration << "\n");
    ShouldContinue = iterateOnFunction(F);
    Changed |= ShouldContinue;
    ++Iteration;
  }
  
  if (EnablePRE) {
    // Fabricate val-num for dead-code in order to suppress assertion in
    // performPRE().
    assignValNumForDeadCode();
    bool PREChanged = true;
    while (PREChanged) {
      PREChanged = performPRE(F);
      Changed |= PREChanged;
    }

Note that GVN above is completed before scalar PRE.

The only case this isn't true is here:

//

  If this load follows a GEP, see if we can PRE the indices before analyzing.
   if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(LI->getOperand(0))) {
     for (GetElementPtrInst::op_iterator OI = GEP->idx_begin(),
                                         OE = GEP->idx_end();
          OI != OE; ++OI)
       if (Instruction *I = dyn_cast<Instruction>(OI->get()))
         performScalarPRE(I);
   }

Obviously, this is broken if we haven't value numbered the predecessors.
In fact, even with your fix, i don't see how it could ever get correct answers.

So i'm confused how your issue occurs at all, and feel like i'm missing something.
Can you give me a code path/example, other than the GEP one (which is not mentioned in your bugzilla bug), where we try to perform scalar PRE before value numbering a predecessor?


https://reviews.llvm.org/D35475