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&id=75261#toc
Repository:
rL LLVM
https://revie
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
test/CodeGen/
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
test/CodeGen/debug-i
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()
> +
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:
lib/CodeG
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 ASTCon
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 st
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
lib/CodeGen/C
c+6ee6274d38fdf...@reviews.llvm.org;
vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com;
mehdi.am...@apple.com
*Cc:* cfe-commits (cfe-commits@lists.llvm.org)
*Subject:* Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to
DIBuilder only if aligment was forced
On Mon, Sep 12,
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 checking
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 t
g)
Subject: 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
mailto:paul.robin...@sony.com>> wrote:
The text in the committee draft is different (e.g., the exhortation about
non-default alignment i
:* Monday, September 12, 2016 5:12 PM
> *To:* reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org;
> vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com;
> mehdi.am...@apple.com
> *Cc:* cfe-commits@lists.llvm.org
> *Subject:* Re: [PATCH] D24426: DebugInfo: Pass non
mehdi.am...@apple.com
Cc: cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder
only if aligment was forced
On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson
mailto:paul.robin...@sony.com>> wrote:
probinson added a subscriber: probinson.
===
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:
On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson
wrote:
> 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;
> ---
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? S
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)
==
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
19 matches
Mail list logo