[DAG] Don't increase SDNodeOrder for dbg.value/declare.

[uabelho]

Summary

The SDNodeOrder is saved in the IROrder field in the SDNode, and this
field may affects scheduling. Thus, letting dbg.value/declare increase
the order numbers may in turn affect scheduling.

Because of this change we also need to update the code deciding when
dbg values should be output, in ScheduleDAGSDNodes.cpp/ProcessSDDbgValues.

Dbg values now have the same order as the SDNode they are connected to,
not the following orders.

Test cases provided by Florian Hahn.

[uabelho]

Ping. Anyone has an opinion about this?

I suppose everyone agrees that dbg.values shouldn't affect code generation?

[probinson]

In D25318#568938, @uabelho wrote:

Ping. Anyone has an opinion about this?

I suppose everyone agrees that dbg.values shouldn't affect code generation?

Unquestionably, debug info should have no effect on code generation.
The question is who feels comfortable enough with SelectionDAG to have an informed opinion about fiddling with this Order stuff. Not me, that's for sure.

[MatzeB]

Yes the presence or absence of DbgValues should not affect the generated (assembly) code.

Do you have any idea why numbering the DbgValues affects the schedule? At a first glance I only found the node order used in comparisons and sorting which shouldn't be affected whether you count the DbgValues or not.

[uabelho]

In D25318#569595, @MatzeB wrote:

Yes the presence or absence of DbgValues should not affect the generated (assembly) code.

Do you have any idea why numbering the DbgValues affects the schedule? At a first glance I only found the node order used in comparisons and sorting which shouldn't be affected whether you count the DbgValues or not.

What I saw when I debugged this in my out-of-tree target was that in ScheduleDAGRRList.cpp, the IROrder is used e.g via bu_ls_rr_sort::operator() (via BURRSort, via SPQ->getNodeOrdering), affecting the order in which the instructions were placed in the final schedule, which in turn affected register allocation.

Turning on -debug printouts when compiling the example in the patch you can see that the result of the scheduler is different when the dbg.value is present.

[probinson]

Could you provide a small C/C++ source that demonstrates the problem using an in-tree target? That is, something where the generated code is different with and without -g, but applying your patch makes the difference go away. It is disappointing but not really surprising that no existing tests are affected.

[uabelho]

In D25318#570397, @probinson wrote:

Could you provide a small C/C++ source that demonstrates the problem using an in-tree target? That is, something where the generated code is different with and without -g, but applying your patch makes the difference go away. It is disappointing but not really surprising that no existing tests are affected.

I can try, but to be honest I think it's demonstrated by the two functions in the ll-file already.
By only adding a dbg.value the resulting code gets different and the difference goes away when applying the patch.

Ideally the whole output of the two functions should be compared though and not just the first pushq.

[fhahn]

I've tested your change and it solves my test case in D27261 as well. This patch is slightly nicer then mine (D27261) imo.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

977

I think you could use if (!isa<DbgInfoIntrinsic>(I)) here.

[aprantl]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

976

and a . at the end of the comment ;-)

977

This would be not just shorter but also more future-proof.

test/DebugInfo/Generic/selectiondag-order.ll

5

Is there a good reason for this? Does it make the testcase more stable?
If we only check the first instruction, sh/could we simplify the IR?

[uabelho]

Glad to see someone else also noticing this problem! :)

We've been running with this patch on our out-of-tree target for two months now without seeing
any problems with it.

I completely agree with the first two comments. I'm not sure what to do with the test case.
Any suggestions? Maybe just use the tests in https://reviews.llvm.org/D27261 instead?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

976

Ok!

977

Good point, I agree!

test/DebugInfo/Generic/selectiondag-order.ll

5

I simply didn't know how to write the test in the best way. I would like to check that the generated instructions are completely identical in the two cases since they only differ in a dbg.value in the input.

Now it's been a while since I saw this problem but I think I tried to reduce more but couldn't if I stil lwanted to see the diff in the output. Someone else might of course be more succesful though.

[uabelho]

Fixed the first two comments. Test case is still the same.

[fhahn]

I've uploaded two test cases in D27261, one with checks for AArch64 and one with checks for X86. I think it would be good to use those, as the test code in this review wasn't problementic on Aarch64. ideally we would have a general test that checks that all produced assembler instructions are equal, but I don't think there's a way to do that with FileCheck (and without pulling in other tools as dependency for the test).

[uabelho]

In D25318#611529, @fhahn wrote:

I've uploaded two test cases in D27261, one with checks for AArch64 and one with checks for X86. I think it would be good to use those, as the test code in this review wasn't problementic on Aarch64.

Sure! Should I simply remove "my" test and add "your" tests in this patch or how do we do it?

ideally we would have a general test that checks that all produced assembler instructions are equal, but I don't think there's a way to do that with FileCheck (and without pulling in other tools as dependency for the test).

Yep! That's kind of how we do it for our out-of-tree target and that's how I found this bug. We compile random C-programs with and without -g and verify that the .text sections in the object files are identical but of course it then pulls in other tools to be able to do that. :/

[fhahn]

In D25318#611530, @uabelho wrote:

In D25318#611529, @fhahn wrote:

I've uploaded two test cases in D27261, one with checks for AArch64 and one with checks for X86. I think it would be good to use those, as the test code in this review wasn't problementic on Aarch64.

Sure! Should I simply remove "my" test and add "your" tests in this patch or how do we do it?

Yes I think the easiest thing would be for you to just add my tests to your review.

ideally we would have a general test that checks that all produced assembler instructions are equal, but I don't think there's a way to do that with FileCheck (and without pulling in other tools as dependency for the test).

Yep! That's kind of how we do it for our out-of-tree target and that's how I found this bug. We compile random C-programs with and without -g and verify that the .text sections in the object files are identical but of course it then pulls in other tools to be able to do that. :/

There's a utility in the clang repo (clang/utils/check_cfc), but I don't think it's used in any tests and we should have CodGen tests for the patch in llvm anyways.

[uabelho]

Used the test cases from https://reviews.llvm.org/D27261

[fhahn]

Ping. Do you think the updated test cases are sufficient or would it help to provide a C example? I have one lying around somewhere, but the test case is quite ugly.

The patch only changes the Order value for debug intrinsics, so that the Order of debug intrinsics is equal to the previous non-debug instruction. The order of non-debug instructions is not affected by the change (as far as I can tell), because all non-debug instruction still have distinct Order values.

The order of debug instructions between two non-debug instructions could change though, because their order now relies on the order returned by DAG->GetDbgValues(N), but this shouldn't be a problem, right?

[fhahn]

Ping. The patch still applies to recent master and fixes a bunch of codegen inconsistencies for the LLVM test-suite. My previous comment gives some details why I think this change should be save to add

[atrick]

In principle this makes sense to me. Go ahead if there are no objections from @bogner or @aprantl.

[uabelho]

Thank you @atrick!

So, @bogner and @aprantl, no objections?

[fhahn]

Awesome, thanks for taking time to review the patch! And thanks @uabelho for your patience :)

lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

 /// ProcessSDDbgValues - Process SDDbgValues associated with this node.
 static void
 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);
       }
       DVs[i]->setIsInvalidated();
     }
   }

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 
 void SelectionDAGBuilder::visit(const Instruction &I) {
   // Set up outgoing PHI node register values before emitting the terminator.
   if (isa<TerminatorInst>(&I)) {
     copySwiftErrorsToFinalVRegs(*this);
     HandlePHINodesInSuccessorBlocks(I.getParent());
   }
 
+  // Increase the SDNodeOrder if dealing with a non-debug instruction
+  if (!isa<DbgValueInst>(I) && !isa<DbgDeclareInst>(I))
     ++SDNodeOrder;
 
   CurInst = &I;
 
   visit(I.getOpcode(), I);
 
   if (!isa<TerminatorInst>(&I) && !HasTailCall &&
       !isStatepoint(&I)) // statepoints handle their exports internally
     CopyToExportRegsIfNeeded(&I);

test/DebugInfo/Generic/selectiondag-order.ll

+;RUN: llc %s -O1 -o - | FileCheck %s
+
+; Functions fn1_dbg and fn1_no_dbg are identical with the exception of one
+; llvm.dbg.value in fn1_dbg. The generated instruction should be identical.
+; However currently we just check the first pushq.
+
+@c = local_unnamed_addr global i16 0
+
+;CHECK-LABEL: @fn1_dbg
+;CHECK: pushq [[REG:.*]]
+define i16 @fn1_dbg() {
+bb1:
+  %_tmp1 = tail call i16 bitcast (i16 (...) * @fn2 to i16 () *)()
+  tail call void @llvm.dbg.value(metadata i16 0, i64 0, metadata !8, metadata !DIExpression()), !dbg !6
+  br label %bb2.preheader.i
+
+bb4.i:
+  %_tmp25.i = add nuw nsw i16 %j.5.08.i, 1
+  %exitcond11.i = icmp eq i16 %_tmp25.i, 4
+  br i1 %exitcond11.i, label %fn3.exit, label %bb2.preheader.i
+
+bb2.preheader.i:
+  %j.5.08.i = phi i16 [ 0, %bb1 ], [ %_tmp25.i, %bb4.i ]
+  br label %bb4.i
+
+fn3.exit:
+  store i16 %_tmp1, i16* @c, align 1
+  ret i16 0
+}
+
+;CHECK-LABEL: @fn1_no_dbg
+;CHECK: pushq [[REG]]
+define i16 @fn1_no_dbg() {
+bb1:
+  %_tmp1 = tail call i16 bitcast (i16 (...) * @fn2 to i16 () *)()
+  br label %bb2.preheader.i
+
+bb4.i:
+  %_tmp25.i = add nuw nsw i16 %j.5.08.i, 1
+  %exitcond11.i = icmp eq i16 %_tmp25.i, 4
+  br i1 %exitcond11.i, label %fn3.exit, label %bb2.preheader.i
+
+bb2.preheader.i:
+  %j.5.08.i = phi i16 [ 0, %bb1 ], [ %_tmp25.i, %bb4.i ]
+  br label %bb4.i
+
+fn3.exit:
+  store i16 %_tmp1, i16* @c, align 1
+  ret i16 0
+}
+
+declare i16 @fn2(...) local_unnamed_addr
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #0
+
+attributes #0 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!3, !4}
+
+!1 = !DIFile(filename: "foo.c", directory: "/tmp")
+!2 = distinct !DICompileUnit(language: DW_LANG_C, file: !1)
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+
+!5 = distinct !DISubprogram(name: "fn1_dbg", unit: !2)
+!6 = distinct !DILocation(line: 8, scope: !5)
+!7 = !DIBasicType(name: "int", size: 16, align: 16, encoding: DW_ATE_signed)
+!8 = !DILocalVariable(name: "j", scope: !5, type: !7)