Page MenuHomePhabricator

[DebugInfo] Adjust fragment offset for big endian targets when splitting alloca in SROA
AcceptedPublic

Authored by Ka-Ka on Feb 14 2019, 1:17 AM.

Details

Summary

When handling debug info fragments of types where padding have been
introduced, the offset have to be adjusted on big endian targets to
cover the correct parts of the value.

Diff Detail

Event Timeline

Ka-Ka created this revision.Feb 14 2019, 1:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 1:17 AM
aprantl added inline comments.Feb 14 2019, 8:35 AM
lib/Transforms/Scalar/SROA.cpp
4279

Since there are many places in the compiler where we create fragments, would it be an option to adapt the semantics of DW_OP_LLVM_fragment in a way that allows us to defer the special handling of big endian targets to AsmPrinter/DwarfExpression.cpp ?
Or, if that doesn't work create an API for creating new fragments that force users to think about what to do on big-endian targets?

Ka-Ka added inline comments.Feb 15 2019, 4:58 AM
lib/Transforms/Scalar/SROA.cpp
4279

If I interpret you correctly you suggest to extend DW_OP_LLVM_fragment to hold additional information about the hole (the undescribed bits between this and the next fragment) that might follow the fragment. That would be a larger change, but it might be worth it.

aprantl added inline comments.Feb 15 2019, 8:25 AM
lib/Transforms/Scalar/SROA.cpp
4279

I think I may need a refresher about what the problem here is. The code here is making the gap between fragments larger, but it's not immediately obvious why. Is that because we are counting the bits from offset 0 to the other end of the value in big endian? I think I may need some ASCII art to illustrate the problem :-)

Ka-Ka added inline comments.Feb 18 2019, 2:16 AM
lib/Transforms/Scalar/SROA.cpp
4279

The code only move where the gap is. The problem here it that the code handle a type that don't fill the entire variable in memory (due to bitfields).

The testcase added to this patch is a copy of X86 testcase the test/DebugInfo/X86/sroasplit-5.ll and adapted to powerpc. It demonstrate how the patch work. The variable in the testcase consist of a struct of a i32 and a i24.

On little endian the variable will look like this in memory:

+--+------+--------+
|XX|  i24 |    i32 |
+--+------+--------+

Th XX repressent a 8-bit gap (padding).

On big endian the variable will look like this in memory:

+------+--+--------+
|  i24 |XX|    i32 |
+------+--+--------+

We need therefore to adjust the offset of the i24-fragment.

As you said above there are many places where we create fragments in the compiler, but I'm not sure all of those places need this kind of special handling of padding as needed here in SROA.

aprantl accepted this revision.Feb 18 2019, 9:47 AM

LGTM with update to testcase.

lib/Transforms/Scalar/SROA.cpp
4279

Thanks :-)

The variable in the testcase is a struct with a single member though? Probably over-reduced.

0  7     31       63
+--+------+--------+
|XX|  i24 |    i32 |
+--+------+--------+
      <-LSB

0     23  31      63
+------+--+--------+
|  i24 |XX|    i32 |
+------+--+--------+
LSB->

I see. Because we don't explicitly describe the gaps in LLVM IR, we can't use a universal addressing scheme. But as you say, SROA is special since it chops up structs differently on big-endian anyway, so doing something special here may be the right solution.

Can you adjust the testcase to match your example and include the ASCII art in a comment explaining what's going on/expected?

This revision is now accepted and ready to land.Feb 18 2019, 9:47 AM