Not increasing SDNodeOrder for llvm.dbg.value to avoid different scheduling?

Hi,

We've noticed a case where ScheduleDAGRRList behaves differently when there are llvm.dbg.value present.

When building the selection dag, the SDNodeOrder field is increased for every visited instruction, including calls to llvm.dbg.value. The SDNodeOrder is saved in each SDNode in the IROrder field.

Then in ScheduleDAGRRList.cpp, the IROrder is used, e.g via bu_ls_rr_sort::operator() (via BURRSort, via SPQ->getNodeOrdering) affecting the scheduling order.

This can result in different scheduling order depending on if there happens to be calls to llvm.dbg.value present in the code or not.

If we only increase the SDNodeNumber for non llvm.dbg.value/declare instructions then this goes away:

+ // Increase the SDNodeOrder if dealing with a non-debug instruction
+ if (!isa<DbgValueInst>(I) && !isa<DbgDeclareInst>(I))
     ++SDNodeOrder;

However, I've no idea if this change is fundamentally breaking how the SDNodeOrder stuff is supposed to work.

Does this seem reasonable?

I noticed that one test case failed due to the change above.

For test/DebugInfo/X86/Output/subregisters.ll I got a few changes in the output which I suppose is a significant regression:

27c27
< DW_AT_location DW_FORM_block1

Hi,

Hi,

We've noticed a case where ScheduleDAGRRList behaves differently when
there are llvm.dbg.value present.

When building the selection dag, the SDNodeOrder field is increased for
every visited instruction, including calls to llvm.dbg.value. The
SDNodeOrder is saved in each SDNode in the IROrder field.

Then in ScheduleDAGRRList.cpp, the IROrder is used, e.g via
bu_ls_rr_sort::operator() (via BURRSort, via SPQ->getNodeOrdering)
affecting the scheduling order.

This can result in different scheduling order depending on if there
happens to be calls to llvm.dbg.value present in the code or not.

If we only increase the SDNodeNumber for non llvm.dbg.value/declare
instructions then this goes away:

+ // Increase the SDNodeOrder if dealing with a non-debug instruction
+ if (!isa<DbgValueInst>(I) && !isa<DbgDeclareInst>(I))
    ++SDNodeOrder;

However, I've no idea if this change is fundamentally breaking how the
SDNodeOrder stuff is supposed to work.

Does this seem reasonable?

I noticed that one test case failed due to the change above.

It seems like I need to update
ProcessSDDbgValues in ScheduleDAGSDNodes.cpp as well:

- if (!Order || DVOrder == ++Order) {
+ if (!Order || DVOrder == Order) {

since I don't increase the SDNodeOrder for llvm.dbg.value/declare, I need to change the check in ProcessSDDbgValues so it searches for dbgValues with the _same_ order as the SDNode, not orders strictly following it.

So with

@@ -971,11 +971,13 @@ void SelectionDAGBuilder::visit(const Instruction &I) {
    if (isa<TerminatorInst>(&I)) {
      copySwiftErrorsToFinalVRegs(*this);
      HandlePHINodesInSuccessorBlocks(I.getParent());
    }

- ++SDNodeOrder;
+ // Increase the SDNodeOrder if dealing with a non-debug instruction
+ if (!isa<DbgValueInst>(I) && !isa<DbgDeclareInst>(I))
+ ++SDNodeOrder;

    CurInst = &I;

    visit(I.getOpcode(), I);

and

@@ -702,20 +702,20 @@ ProcessSDDbgValues(SDNode *N, SelectionDAG *DAG, InstrEmitter &Emitter,
                     SmallVectorImpl<std::pair<unsigned, MachineInstr*> > &Orders,
                     DenseMap<SDValue, unsigned> &VRBaseMap, unsigned Order) {
    if (!N->getHasDebugValue())
      return;

- // Opportunistically insert immediate dbg_value uses, i.e. those with source
- // order number right after the N.
+ // Opportunistically insert immediate dbg_value uses, i.e. those with the same
+ // source order number as N.
    MachineBasicBlock *BB = Emitter.getBlock();
    MachineBasicBlock::iterator InsertPos = Emitter.getInsertPos();
    ArrayRef<SDDbgValue*> DVs = DAG->GetDbgValues(N);
    for (unsigned i = 0, e = DVs.size(); i != e; ++i) {
      if (DVs[i]->isInvalidated())
        continue;
      unsigned DVOrder = DVs[i]->getOrder();
- if (!Order || DVOrder == ++Order) {
+ if (!Order || DVOrder == Order) {
        MachineInstr *DbgMI = Emitter.EmitDbgValue(DVs[i], VRBaseMap);
        if (DbgMI) {
          Orders.push_back(std::make_pair(DVOrder, DbgMI));
          BB->insert(InsertPos, DbgMI);
        }

the problem where llvm.dbg.value affected scheduling goes away, and all tests in test/ passes.

Question remains, is this fundamentaly breaking how the orders are supposed to work or is this change sane?

Appreciating any input,
Mikael