One of your run lines doesn't do anything: +// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s
Please remove it. On Fri, May 1, 2015 at 12:10 PM, Li, Charles < [email protected]> wrote: > Hi David, > > > > I have revised the TLS Max Align patch taken into account of all the > reviews from you and Richard. > > I would very much appreciate another code review. > > > > Also Richard has the following question for you from the previous email > (review for the 3rd revision of the patch). I would also appreciate your > input. > > > David: You have the most recent state on this -- can you check this is > doing the right set of checks to find a dependent alignment? (Also, are we > doing this check elsewhere? Should we move this check to ASTContext?) > > > > Thank you, > > Charles Li > > > > > > *From:* Li, Charles > *Sent:* Thursday, April 30, 2015 6:48 PM > *To:* [email protected]; 'Richard Smith'; David Majnemer > *Cc:* Domizioli, Dario > *Subject:* RE: Proposed patch (4th revision) adds TLS Max Alignment > diagnostic > > > > Hi Richard, > > > > Thank you very much for pointing out all the intricacies I was not aware > of. > > Both points have been addressed. > > > > 1. Dependent type check has been moved into the function > hasDependentAlignment > > > > 2. The previous comment > > /// hasDependentAlignment - determines if a variable's alignment is > dependent > > has been changed to > > /// \brief Determines if a variable's alignment is dependent. > > > > Please let me know if there are any more bugs. I very much appreciate it. > > > > Sincerely, > > Charles Li > > > > > > *From:* [email protected] [mailto:[email protected] <[email protected]>] *On > Behalf Of *Richard Smith > *Sent:* Thursday, April 30, 2015 4:43 PM > *To:* Li, Charles; David Majnemer > *Cc:* [email protected]; Domizioli, Dario > *Subject:* Re: Proposed patch (3rd revision) adds TLS Max Alignment > diagnostic > > > > Looks OK to me, but I'd prefer the VD->getType()->isDependentType() check > to be inside hasDependentAlignment, since a variable with a dependent type > is assumed to have a dependent alignment. > > > > David: You have the most recent state on this -- can you check this is > doing the right set of checks to find a dependent alignment? (Also, are we > doing this check elsewhere? Should we move this check to ASTContext?) > > > > One totally trivial thing to fix before commit: > > > > +/// hasDependentAlignment - determines if a variable's alignment is > dependent > > > > We prefer to use \brief instead of repeating the function name in new code. > > > > Thanks! > > > > On Thu, Apr 30, 2015 at 4:21 PM, Li, Charles < > [email protected]> wrote: > > Hi Everyone, > > > > I am back again with the 3rd revision of the patch which adds an error > diagnostic for when TLS variables exceed maximum TLS alignment. > > > > This patch fixed 4 shortcomings. > > > > 1. Dependent alignment check has been spun off into a static > function called hasDependentAlignment > > 2. Redundant code on checking for reference type and getting its > pointee type has been removed. > > 3. Improved diagnostic to show the requested alignment of the > variable > > 4. Added a Run line in the test to ensure x86_64-linux-gnu works the > same as before. > > > > > > Many thanks to the peer reviewers out there. > > > > Sincerely, > > Charles Li > > > > > > *From:* [email protected] [mailto:[email protected]] *On Behalf Of *Richard > Smith > *Sent:* Thursday, April 16, 2015 4:41 PM > *To:* Li, Charles > *Cc:* [email protected]; Domizioli, Dario > *Subject:* Re: Proposed patch (2nd revision) adds TLS Max Alignment > diagnostic > > > > + if (const unsigned MaxAlign = Context.getTargetInfo().getMaxTLSAlign()) > { > > + for (auto *I : VD->specific_attrs<AlignedAttr>()) { > > + if (I->isAlignmentDependent()) > > + return; > > > > Do not return here. You're in the middle of a function that does a bunch > of other checks after this point. Perhaps factor out a static > hasDependentAlignment helper function? > > > > + } > > + > > + if (VD->getTLSKind()) { > > + QualType T = VD->getType(); > > + if (const ReferenceType *RT = T->getAs<ReferenceType>()) > > + T = Context.getPointerType(RT->getPointeeType()); > > > > This code is redundant. The only place you use T is when you detect > whether it's dependent: > > > > + if (!T->isDependentType()) { > > > > ... which is unaffected by whether it's a pointer or a reference type. > > > > > > +def err_tls_var_aligned_over_maximum : Error< > > + "alignment of thread-local variable %0 is greater than the maximum > supported " > > + "alignment (%1) for a thread-local variable on this target">; > > > > Maybe include the requested alignment of the variable in this error? > > > > > > +// RUN: %clang_cc1 -triple x86_64-scei-ps4 -fsyntax-only -verify %s > > > > Please also add something like > > > > // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s > > > > ... to verify that we don't emit any diagnostics for the case where we > don't have an alignment limit. > > > > On Wed, Apr 15, 2015 at 4:04 PM, Li, Charles < > [email protected]> wrote: > > Hi Everyone, > > > > We have updated the proposed patch which adds an error diagnostic for when > TLS variables exceed maximum TLS alignment. > > This update takes care of the case where alignment is dependent. It also > removed the unnecessary getBaseElementType() call. > > > > Sincerely, > > Charles Li > > > > > > > > *From:* Li, Charles > *Sent:* Friday, April 03, 2015 3:21 PM > *To:* 'David Majnemer' > *Cc:* [email protected] > *Subject:* RE: Proposed patch adds TLS Max Alignment diagnostic > > > > Hi David, > > > > > > Oops, we didn’t think of the case where the alignment could be a template > parameter. > > I currently don’t have a clue to fix this. > > Any hint would be greatly welcome. > > > > On why we used BaseT instead of T. > > I think we originally added > > QualType BaseT = Context.getBaseElementType(T); > > Just be to conservative as we don’t fully understand how LLVM’s type > system when it comes to templates. > > I have re-ran clang replacing BaseT with T and did not notice any > regressions. > > So I will take out BaseT for the next revision of the patch. > > > > > > Cheers, > > Charles Li > > > > > > *From:* David Majnemer [mailto:[email protected] > <[email protected]>] > *Sent:* Wednesday, April 01, 2015 7:08 PM > *To:* Li, Charles > *Cc:* [email protected] > *Subject:* Re: Proposed patch adds TLS Max Alignment diagnostic > > > > Hi Charles, > > > > Your patch doesn't handle cases where the alignment is dependent, > getDeclAlign doesn't want to be called in such cases: > > template <int N> > > struct S { > > static int __thread __attribute__((aligned(N))) x; > > }; > > > > Also, why do you use BaseT->isDependentType() instead of > T->isDependentType()? > > > > On Wed, Apr 1, 2015 at 4:57 PM, Li, Charles < > [email protected]> wrote: > > Hi Clang developers, > > > > We here at Sony PlayStation have a proposed patch which adds an error > diagnostic for when TLS variables exceed maximum TLS alignment. > > Please note this patch does not affect normal maximum alignments. > > This TLS maximum alignment check is currently only turned on for PS4 but > could potentially be used for other platforms. > > > > Sincerely, > > Charles Li > > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
