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

2016-09-13 Thread Victor Leschuk via cfe-commits
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

2016-09-13 Thread Victor Leschuk via cfe-commits
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

2016-09-13 Thread Robinson, Paul via cfe-commits
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

2016-09-12 Thread David Blaikie via cfe-commits
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

2016-09-12 Thread Robinson, Paul via cfe-commits
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

2016-09-12 Thread Reid Kleckner via cfe-commits
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

2016-09-12 Thread Paul Robinson via cfe-commits
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

2016-09-12 Thread David Blaikie via cfe-commits
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

2016-09-11 Thread Victor Leschuk via cfe-commits
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