[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

This revision was automatically updated to reflect the committed changes. Closed by commit rL284679: DebugInfo: pass alignment value only if it was forced (authored by vleschuk). Changed prior to commit: https://reviews.llvm.org/D24426?vs=74686=75261#toc Repository: rL LLVM

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk updated this revision to Diff 74686. vleschuk added a comment. Use DIAlignment type instead of uint64_t for alignment in DebugInfo. https://reviews.llvm.org/D24426 Files: lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.h test/CodeGen/debug-info-packed-struct.c

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk updated this revision to Diff 73537. vleschuk added a comment. Pass alignment in bits instead of bytes to backend (conversion is done when emitting DW_AT_alignment). https://reviews.llvm.org/D24426 Files: lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.h

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

rnk accepted this revision. rnk added a reviewer: rnk. rnk added a comment. This revision is now accepted and ready to land. lgtm > CGDebugInfo.cpp:54-56 > + auto TI = Ctx.getTypeInfo(Ty); > + return TI.AlignIsRequired ? Ctx.toCharUnitsFromBits(TI.Align).getQuantity() > +

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk updated this revision to Diff 73318. vleschuk added a comment. Fix code style: - Two overloaded methods instead of 1 template - lower-case-headed method names - static methods instead of anon namespace - extra new line between methods https://reviews.llvm.org/D24426 Files:

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

rnk added inline comments. > CGDebugInfo.cpp:47 > > +namespace { > +template LLVM prefers `static` to anonymous namespaces http://llvm.org/docs/CodingStandards.html#anonymous-namespaces > CGDebugInfo.cpp:48 > +namespace { > +template > +uint64_t GetTypeAlignIfRequired(Type Ty, const

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk marked 3 inline comments as done. vleschuk added inline comments. > vleschuk wrote in CGDebugInfo.cpp:608 > Will check if this works in all cases. I think it's worth putting this > snippet into helper function within anon namespace. That is correct for types, for particular decls we

[PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk updated this revision to Diff 73228. vleschuk added a comment. - Move alignment detection into helper functions in anon namespace - Use updated LLVM DIBuilder API (from https://reviews.llvm.org/D24425) https://reviews.llvm.org/D24426 Files: lib/CodeGen/CGDebugInfo.cpp

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:608 @@ -608,2 +607,3 @@ uint64_t Size = CGM.getContext().getTypeSize(Ty); + uint64_t Align = 0; rnk wrote: > IMO this is what we should be doing everywhere, rather than manually

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3691 @@ -3635,1 +3690,3 @@ + if (D->hasAttr()) +AlignInBits = D->getMaxAlignment(); StringRef DeclName, LinkageName; probinson wrote: > dblaikie wrote: > > is max alignment the right

RE: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

I hadn't thought Clang wanted to be *quite* so knowledgeable about targets, and similarly not so tightly tied to byte-addressable targets. But if both of those things are actually okay, then it's fine to set the alignment value here to what would be passed through to DWARF. --paulr From:

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

On Mon, Sep 12, 2016 at 6:01 PM Robinson, Paul wrote: > The text in the committee draft is different (e.g., the exhortation about > non-default alignment is gone), with an example to the effect that a value > of 8 means the entity's address is a multiple of 8 (not 2^8).

RE: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

The text in the committee draft is different (e.g., the exhortation about non-default alignment is gone), with an example to the effect that a value of 8 means the entity's address is a multiple of 8 (not 2^8). So, alignment is conceived in terms of address bits, whatever those represent (not

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

rnk added a subscriber: rnk. Comment at: lib/CodeGen/CGDebugInfo.cpp:608 @@ -608,2 +607,3 @@ uint64_t Size = CGM.getContext().getTypeSize(Ty); + uint64_t Align = 0; IMO this is what we should be doing everywhere, rather than manually checking AlignedAttr:

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

probinson added a subscriber: probinson. Comment at: lib/CodeGen/CGDebugInfo.cpp:3691 @@ -3635,1 +3690,3 @@ + if (D->hasAttr()) +AlignInBits = D->getMaxAlignment(); StringRef DeclName, LinkageName; dblaikie wrote: > is max alignment the right thing here?

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

dblaikie added a comment. Like the backend patch, should/could this be broken up into separate patches for the different places/features that have alignment? (probably just passing zero for alignment in general could be a simple cleanup patch to start, if it's otherwise unused)

Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

vleschuk retitled this revision from "DebugInfo: use llvm::DINode::FlagAlignment to mark forcibly aligned data" to "DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced". vleschuk updated the summary for this revision. vleschuk updated this revision to Diff 70965. vleschuk