llvm dev - Oct 2016 - 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
---
 >       DW_AT_location  DW_FORM_data4

[...]

109c108
<                   DW_AT_location [DW_FORM_block1]     (<0x01> 55 )
---
 >                   DW_AT_location [DW_FORM_data4]      (0x00000000)

[...]

115,116c114
< 0x00000055:     DW_TAG_variable [4]
<                   DW_AT_location [DW_FORM_data4]      (0x00000000)
---
 > 0x00000057:     DW_TAG_variable [4]

[...]

178,180c176,178
< 0x00000000: Beginning address offset: 0x0000000000000002
<                Ending address offset: 0x0000000000000014
<                 Location description: 54 93 04
---
 > 0x00000000: Beginning address offset: 0x0000000000000000
 >                Ending address offset: 0x0000000000000009
 >                 Location description: 55

Any thoughts on this?

Thanks,
Mikael

Hi,

On 10/05/16 16:02, Mikael Holmén via llvm-dev wrote:
> 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
>
> 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
> ---
>>       DW_AT_location  DW_FORM_data4
>
> [...]
>
> 109c108
> <                   DW_AT_location [DW_FORM_block1]     (<0x01> 55
)
> ---
>>                   DW_AT_location [DW_FORM_data4]      (0x00000000)
>
> [...]
>
> 115,116c114
> < 0x00000055:     DW_TAG_variable [4]
> <                   DW_AT_location [DW_FORM_data4]      (0x00000000)
> ---
>> 0x00000057:     DW_TAG_variable [4]
>
> [...]
>
> 178,180c176,178
> < 0x00000000: Beginning address offset: 0x0000000000000002
> <                Ending address offset: 0x0000000000000014
> <                 Location description: 54 93 04
> ---
>> 0x00000000: Beginning address offset: 0x0000000000000000
>>                Ending address offset: 0x0000000000000009
>>                 Location description: 55
>
> Any thoughts on this?
>
> Thanks,
> Mikael
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev