This is an archive of the discontinued LLVM Phabricator instance.

[Debug info] Handle endianness when moving debug info for split integer values
ClosedPublic

Authored by dstenb on Sep 22 2017, 4:24 AM.

Details

Summary

Take the target's endianness into account when splitting the
debug information in DAGTypeLegalizer::SetExpandedInteger.

This patch fixes so that, for big-endian targets, the fragment
expression corresponding to the high part of a split integer
value is placed at offset 0, in order to correctly represent
the memory address order.

I have attached a PPC32 reproducer where the resulting DWARF
pieces for a 64-bit integer were incorrectly reversed.

Diff Detail

Event Timeline

dstenb created this revision.Sep 22 2017, 4:24 AM
JDevlieghere accepted this revision.Sep 25 2017, 1:09 AM

Thanks David!

LGTM, but maybe wait for the other to have a look too before you land this.

PS: When uploading a diff, please include as much context as you can. It makes reviewing a lot easier.

This revision is now accepted and ready to land.Sep 25 2017, 1:09 AM

LGTM, but maybe wait for the other to have a look too before you land this.

Yeah, I will wait.

PS: When uploading a diff, please include as much context as you can. It makes reviewing a lot easier.

I'm sorry about that! Friday afternoon absent-mindedness...

dblaikie added inline comments.Sep 25 2017, 8:59 AM
test/CodeGen/PowerPC/debuginfo-split-int.ll
19–30

Could you simplify this to:

unsigned long long foo();
void bar() {
  volatile unsigned long long result = foo();
}
aprantl accepted this revision.Sep 25 2017, 9:19 AM

Seems plausible.

dstenb updated this revision to Diff 116710.Sep 26 2017, 2:21 PM

Simplified the test case according to dblaikie's comment (thanks!).

dstenb marked an inline comment as done.Sep 26 2017, 2:28 PM
dblaikie accepted this revision.Sep 27 2017, 11:08 AM

Sounds good, I think.

This revision was automatically updated to reflect the committed changes.
timshen added a subscriber: timshen.Oct 2 2017, 3:27 PM

Hi,

The test doesn't pass on a llc built without the target "AMDGPU" enabled. To reproduce:

  1. Add -DLLVM_TARGETS_TO_BUILD="X86;PowerPC" to the cmake command.
  2. re-run cmake and build llc. The test doesn't pass on that llc.

Notice that if we define LLVM_TARGETS_TO_BUILD as "X86;PowerPC;AMDGPU", the test passes again.

I think this is related to [1], but I don't understand why "isel" defined in AMDGPU gets picked up by a PPC32 through "-stop-after=isel".

[1] https://github.com/llvm-mirror/llvm/blob/7287fcb5d580e8ac0b860ddd4da03dc395795fa0/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp#L237

bjope added a subscriber: bjope.Oct 3 2017, 2:27 AM

Hi,

The test doesn't pass on a llc built without the target "AMDGPU" enabled. To reproduce:

  1. Add -DLLVM_TARGETS_TO_BUILD="X86;PowerPC" to the cmake command.
  2. re-run cmake and build llc. The test doesn't pass on that llc.

Notice that if we define LLVM_TARGETS_TO_BUILD as "X86;PowerPC;AMDGPU", the test passes again.

I think this is related to [1], but I don't understand why "isel" defined in AMDGPU gets picked up by a PPC32 through "-stop-after=isel".

[1] https://github.com/llvm-mirror/llvm/blob/7287fcb5d580e8ac0b860ddd4da03dc395795fa0/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp#L237

Ok, thanks for the revert Tim. I'll reapply this later today by using -stop-before=expand-isel-pseudos instead of -stop-after=isel.
(a bummer that AMDGPU is using such a general name as "isel" when registering the target specific pass... kind of fooled us to think that we could stop after "isel" for any target...)