This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Remove debug locations from ConstantSD(FP)Nodes
ClosedPublic

Authored by vsk on Jun 21 2018, 6:26 PM.

Details

Summary

This removes debug locations from ConstantSDNode and ConstantSDFPNode.

When this kind of node is materialized we no longer create a line table
entry which jumps back to the constant's first point of use. This makes
single-stepping behavior smoother, and it matches the model used by IR,
where Constants have no locations. See this thread for more context:

http://lists.llvm.org/pipermail/llvm-dev/2018-June/124164.html

I'd like to handle constant BuildVectorSDNodes and to try to eliminate
passing SDLocs to SelectionDAG::getConstant*() in follow-up commits.

Diff Detail

Event Timeline

vsk created this revision.Jun 21 2018, 6:26 PM
echristo accepted this revision.Jun 22 2018, 2:46 PM

LGTM.

This revision is now accepted and ready to land.Jun 22 2018, 2:46 PM
This revision was automatically updated to reflect the committed changes.

Hi Vedant,

This patch broke ARM bots since expected line info is missing in
address sanitizer output for null_redef.cc testcase, notice that when
the test is compiled in thumb mode the line info is still present.

logs available at:

http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/1203/steps/ninja%20check%201/logs/FAIL%3A%20AddressSanitizer-armhf-linux%3A%3Anull_deref.cc

Thanks
Yvan

Hi Vedant,

This change also made debug information for the following example slightly worse:

$ cat test.c
void bar(void);
int foo(void) {
  bar();
  int i = 2;
  return i;
}
$ clang -target armv7a-none-eabi -g -c test.c main.c
$ llvm-objdump -S test.o

test.o: file format ELF32-arm-little

Disassembly of section .text:
foo:
; int foo(void) {
       0:       00 48 2d e9     push    {r11, lr}
       4:       0d b0 a0 e1     mov     r11, sp
       8:       08 d0 4d e2     sub     sp, sp, #8
; bar();
       c:       fe ff ff eb     bl      #-8 <foo+0xc>
      10:       02 e0 a0 e3     mov     lr, #2
; int i = 2;
      14:       04 e0 8d e5     str     lr, [sp, #4]
; return i;
      18:       04 00 9d e5     ldr     r0, [sp, #4]
      1c:       0b d0 a0 e1     mov     sp, r11
      20:       00 88 bd e8     pop     {r11, pc}

Instruction mov lr, #2 is recorded as part of line 3 (bar();) but previously it was more accurately accounted as line 4 (int i = 2;).

Previous behaviour:

$ llvm-objdump -S test.o

test.o: file format ELF32-arm-little

Disassembly of section .text:
foo:
; int foo(void) {
       0:       00 48 2d e9     push    {r11, lr}
       4:       0d b0 a0 e1     mov     r11, sp
       8:       08 d0 4d e2     sub     sp, sp, #8
; bar();
       c:       fe ff ff eb     bl      #-8 <foo+0xc>
; int i = 2;
      10:       02 e0 a0 e3     mov     lr, #2
      14:       04 e0 8d e5     str     lr, [sp, #4]
; return i;
      18:       04 00 9d e5     ldr     r0, [sp, #4]
      1c:       0b d0 a0 e1     mov     sp, r11
      20:       00 88 bd e8     pop     {r11, pc}

It is not clear to me whether this is an expected change but I thought I would let you know about it if it actually isn't.

Thanks,
Petr

vsk added a comment.Jun 29 2018, 9:28 AM

From @yroux:

Hi Vedant,

This patch broke ARM bots since expected line info is missing in
address sanitizer output for null_redef.cc testcase, notice that when
the test is compiled in thumb mode the line info is still present.

logs available at:

http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/1203/steps/ninja%20check%201/logs/FAIL%3A%20AddressSanitizer-armhf-linux%3A%3Anull_deref.cc

Thanks
Yvan

Phab didn't pick up my reply from the mailing list, so just to recap: thanks for bringing this to my attention -- I've relaxed the test and the bot has recovered.

Hi Vedant,

This change also made debug information for the following example slightly worse:

$ cat test.c
void bar(void);
int foo(void) {
  bar();
  int i = 2;
  return i;
}
$ clang -target armv7a-none-eabi -g -c test.c main.c
$ llvm-objdump -S test.o

test.o: file format ELF32-arm-little

Disassembly of section .text:
foo:
; int foo(void) {
       0:       00 48 2d e9     push    {r11, lr}
       4:       0d b0 a0 e1     mov     r11, sp
       8:       08 d0 4d e2     sub     sp, sp, #8
; bar();
       c:       fe ff ff eb     bl      #-8 <foo+0xc>
      10:       02 e0 a0 e3     mov     lr, #2
; int i = 2;
      14:       04 e0 8d e5     str     lr, [sp, #4]
; return i;
      18:       04 00 9d e5     ldr     r0, [sp, #4]
      1c:       0b d0 a0 e1     mov     sp, r11
      20:       00 88 bd e8     pop     {r11, pc}

Instruction mov lr, #2 is recorded as part of line 3 (bar();) but previously it was more accurately accounted as line 4 (int i = 2;).

Previous behaviour:

$ llvm-objdump -S test.o

test.o: file format ELF32-arm-little

Disassembly of section .text:
foo:
; int foo(void) {
       0:       00 48 2d e9     push    {r11, lr}
       4:       0d b0 a0 e1     mov     r11, sp
       8:       08 d0 4d e2     sub     sp, sp, #8
; bar();
       c:       fe ff ff eb     bl      #-8 <foo+0xc>
; int i = 2;
      10:       02 e0 a0 e3     mov     lr, #2
      14:       04 e0 8d e5     str     lr, [sp, #4]
; return i;
      18:       04 00 9d e5     ldr     r0, [sp, #4]
      1c:       0b d0 a0 e1     mov     sp, r11
      20:       00 88 bd e8     pop     {r11, pc}

It is not clear to me whether this is an expected change but I thought I would let you know about it if it actually isn't.

Thanks,
Petr

Hi @petpav01, the behavior difference you're seeing is an expected result of this patch. The tradeoff is to give up some debug info precision on single-use constant materialization in order to improve single-stepping when constants are reused. I'm happy to try and find a better solution if this regresses some behavior you depend on.

In other words, the mov lr, #2 is the *materialization* of the constant and is no longer associated with a specific source location.

yroux added a comment.Jul 2 2018, 1:07 AM

Thanks for the fix Vedant.

Thanks for the explanation. This does not cause any immediate problem for us but it is unfortunate that a simple assignment of a constant now appears to be split between two lines. Hopefully it will be potentially possible to implement some improvement for this case in the future.

test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll