[llvm-dev] Not increasing SDNodeOrder for llvm.dbg.value to avoid different scheduling?

[Mikael Holmén via llvm-dev]

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