[LLVMdev] MachineSink.cpp bug?

[Jonas Paulsson]

Hi,

In method MachineSinking::PostponeSplitCriticalEdge(), in the last section it says:

  // It's only legal to break critical edge and sink the computation to the
  // new block if all the predecessors of "To", except for "From", are
  // not dominated by "From". Given SSA property, this means these
  // predecessors are dominated by "To".
  //
  // There is no need to do this check if all the uses are PHI nodes. PHI
  // sources are only defined on the specific predecessor edges.
  if (!BreakPHIEdge) {
    for (MachineBasicBlock::pred_iterator PI = ToBB->pred_begin(),
           E = ToBB->pred_end(); PI != E; ++PI) {
      if (*PI == FromBB)
        continue;
      if (!DT->dominates(ToBB, *PI))
        return false;
    }
  }

I find this a bit strange and suspect an error, since the loop checks if ToBB dominates all its predecessors.
The only case I can think of is a one block loop...

I wonder what "Given SSA property, this means these predecessors are dominated by "To"" means.

Could it perhaps be rewritten to:

    for (MachineBasicBlock::pred_iterator PI = ToBB->pred_begin(),
           E = ToBB->pred_end(); PI != E; ++PI) {
      if (*PI == FromBB)
        continue;
      if (DT->dominates(FromBB, *PI))
        return false;
    }

?

I also wonder by the way, why MachineSink bothers to split blocks and why there is a FIXME comment about sinking
instructions lower in the same block. Isn't that the job of the mischeduler, to put those instructions
near the end of the block if there are no local uses?

Best regards,

Jonas Paulsson

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141110/57f694a5/attachment.html>