[PATCH] D58224: [DebugInfo] Adjust fragment offset for big endian targets when splitting alloca in SROA

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

LGTM with update to testcase.



================
Comment at: lib/Transforms/Scalar/SROA.cpp:4279
+          Offset += std::max(AllocaSize, P.size() * SizeOfByte) - Size;
+        Fragments.push_back(Fragment(NewAI, Offset, Size));
       }
----------------
Ka-Ka wrote:
> aprantl wrote:
> > Ka-Ka wrote:
> > > aprantl wrote:
> > > > 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?
> > > 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.
> > 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 :-)
> 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.
> 
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?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58224/new/

https://reviews.llvm.org/D58224