llvm dev - Oct 2016 - DebugInfo: purpose of align field

Hello Adrian, sorry for the delay with the response. Somehow I missed 
your message..

On 09/13/2016 12:43 AM, Adrian Prantl wrote:
>> On Sep 10, 2016, at 12:50 PM, Victor Leschuk via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
>>
>> Hello all,
>>
>> I am currently implementing support for DWARFv5 DW_AT_alignment attr
and I got a question about align field in debug info section of IR/Bitcode.
> Thanks for looking into this!
No problem =)
>
>> Currently it is being dumped almost in any case, however according to
code we use align from DI* objects only when dealing with class/structure
bitfields: DwarfUnit::constructMemberDIE.
>>
>> Dumping align information everywhere only for 1 case looks like
overhead to me.
> One your patch is finished it will be used in more cases, right? :-)
My point is that we dump align information everywhere and it is 
currently used only in 1 case. I am trying to make it so that it is 
dumped only when it is necessary.
>> Consider the following code:
>>
>> struct S {
>>   char c;
>> } s;
>>
>> When compiled with debug enabled in IR we get smth like that:
>>
>> !6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name:
"S", file: !5, line: 1, size: 8, align: 8, elements: !7)
>>
>> Am I missing something? What is the purpose of "align: 8"
here? Maybe we could include alignment information into DI* objects only when
required (and thus dump it only when required): for types with bitfields and
when alignas() was specified in code.
> What do you mean by "include" in this context? Are you arguing
for making an alignment of 8 the implicit default in textual IR? Or do you want
to make DICompositeType variable length?
I mean that in this case alignment isn't used anywhere in code further. 
I think we don't need alignment information in textual DI IR in case it 
is not default. Dumping default alignment in every case doesn't make 
sense for me: if we dump align info each time we need to mark
"special"
cases with additional flag in order to be able to know which case to 
"report" in DWARF. If we dump only "forced" alignment, we
can just say:
"Ok, alignment is specified, we need to put this into
DWARF".
>
> -- adrian
>
>> Please advise.
>>
>> -- 
>> Best Regards,
>> Victor
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Best Regards,
Victor

> On Sep 30, 2016, at 4:05 PM, Victor Leschuk <vleschuk at
accesssoftek.com> wrote:
> 
> Hello Adrian, sorry for the delay with the response. Somehow I missed your
message..
> 
> On 09/13/2016 12:43 AM, Adrian Prantl wrote:
>>> On Sep 10, 2016, at 12:50 PM, Victor Leschuk via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>>> 
>>> Hello all,
>>> 
>>> I am currently implementing support for DWARFv5 DW_AT_alignment
attr and I got a question about align field in debug info section of IR/Bitcode.
>> Thanks for looking into this!
> No problem =)
>> 
>>> Currently it is being dumped almost in any case, however according
to code we use align from DI* objects only when dealing with class/structure
bitfields: DwarfUnit::constructMemberDIE.
>>> 
>>> Dumping align information everywhere only for 1 case looks like
overhead to me.
>> One your patch is finished it will be used in more cases, right? :-)
> My point is that we dump align information everywhere and it is currently
used only in 1 case. I am trying to make it so that it is dumped only when it is
necessary.
>>> Consider the following code:
>>> 
>>> struct S {
>>>  char c;
>>> } s;
>>> 
>>> When compiled with debug enabled in IR we get smth like that:
>>> 
>>> !6 = distinct !DICompositeType(tag: DW_TAG_structure_type, name:
"S", file: !5, line: 1, size: 8, align: 8, elements: !7)
>>> 
>>> Am I missing something? What is the purpose of "align: 8"
here? Maybe we could include alignment information into DI* objects only when
required (and thus dump it only when required): for types with bitfields and
when alignas() was specified in code.
>> What do you mean by "include" in this context? Are you
arguing for making an alignment of 8 the implicit default in textual IR? Or do
you want to make DICompositeType variable length?
> I mean that in this case alignment isn't used anywhere in code further.
I think we don't need alignment information in textual DI IR in case it is
not default. Dumping default alignment in every case doesn't make sense for
me: if we dump align info each time we need to mark "special" cases
with additional flag in order to be able to know which case to
"report" in DWARF. If we dump only "forced" alignment, we
can just say: "Ok, alignment is specified, we need to put this into
DWARF".
I'm not sure I understand your point.

In textual IR (lib/IR/AsmWriter.cpp & friends) we generally don't care
too much about how verbose the output is, since this representation is primarily
used for debugging.
>From how you phrased your reply it is also possible that you were instead
thinking about the in-memory representation? Currently every DIType
(include/IR/DebugInfoMetadata.h) uses a unit64_t field to store the alignment.
IIUC, your goal is to avoid emitting a DW_AT_alignment attribute to types that
use the default alignment. Would it make sense to store the target's default
alignment in the DIType::AlignInBits field, so the DWARF backend can then decide
not emit the DW_AT_alignment when it sees it?

-- adrian

On Mon, Oct 3, 2016 at 2:24 PM Adrian Prantl via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> > On Sep 30, 2016, at 4:05 PM, Victor Leschuk <vleschuk at
accesssoftek.com>
> wrote:
> >
> > Hello Adrian, sorry for the delay with the response. Somehow I missed
> your message..
> >
> > On 09/13/2016 12:43 AM, Adrian Prantl wrote:
> >>> On Sep 10, 2016, at 12:50 PM, Victor Leschuk via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >>>
> >>> Hello all,
> >>>
> >>> I am currently implementing support for DWARFv5
DW_AT_alignment attr
> and I got a question about align field in debug info section of IR/Bitcode.
> >> Thanks for looking into this!
> > No problem =)
> >>
> >>> Currently it is being dumped almost in any case, however
according to
> code we use align from DI* objects only when dealing with class/structure
> bitfields: DwarfUnit::constructMemberDIE.
> >>>
> >>> Dumping align information everywhere only for 1 case looks
like
> overhead to me.
> >> One your patch is finished it will be used in more cases, right?
:-)
> > My point is that we dump align information everywhere and it is
> currently used only in 1 case. I am trying to make it so that it is dumped
> only when it is necessary.
> >>> Consider the following code:
> >>>
> >>> struct S {
> >>>  char c;
> >>> } s;
> >>>
> >>> When compiled with debug enabled in IR we get smth like that:
> >>>
> >>> !6 = distinct !DICompositeType(tag: DW_TAG_structure_type,
name: "S",
> file: !5, line: 1, size: 8, align: 8, elements: !7)
> >>>
> >>> Am I missing something? What is the purpose of "align:
8" here? Maybe
> we could include alignment information into DI* objects only when required
> (and thus dump it only when required): for types with bitfields and when
> alignas() was specified in code.
> >> What do you mean by "include" in this context? Are you
arguing for
> making an alignment of 8 the implicit default in textual IR? Or do you want
> to make DICompositeType variable length?
> > I mean that in this case alignment isn't used anywhere in code
further.
> I think we don't need alignment information in textual DI IR in case it
is
> not default. Dumping default alignment in every case doesn't make sense
for
> me: if we dump align info each time we need to mark "special"
cases with
> additional flag in order to be able to know which case to
"report" in
> DWARF. If we dump only "forced" alignment, we can just say:
"Ok, alignment
> is specified, we need to put this into DWARF".
>
> I'm not sure I understand your point.
>
> In textual IR (lib/IR/AsmWriter.cpp & friends) we generally don't
care too
> much about how verbose the output is, since this representation is
> primarily used for debugging.
>
> From how you phrased your reply it is also possible that you were instead
> thinking about the in-memory representation? Currently every DIType
> (include/IR/DebugInfoMetadata.h) uses a unit64_t field to store the
> alignment.
>
> IIUC, your goal is to avoid emitting a DW_AT_alignment attribute to types
> that use the default alignment. Would it make sense to store the
target's
> default alignment in the DIType::AlignInBits field, so the DWARF backend
> can then decide not emit the DW_AT_alignment when it sees it?
>
This whole thread's got a bit long/confusing for me, but some of what's
going on might be suggestions I made earlier. My rough idea is that:

1) We should probably only put alignment on metadata nodes that we want the
DW_AT_alignment attribute to be generated for (keeps the model simple) -
leave it zero otherwise.
2) It'd be nice not to print the zero, to keep the textual representation
simple/less noisy (as we do with many other metadata fields)

But it's not a big deal if someone else feels strongly.

- Dave

>
> -- adrian
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161003/3a3253e1/attachment-0001.html>