[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D147655#4650964 , @probinson wrote:

> Hi @rsmith,
>
>> these two different templates would have the same mangling:
>
>   template  T returnit() {return I;};
>   template  T returnit() { return I; }
>
> I tried compiling `long foo() { return returnit(); }` with these two 
> templates, and got different manglings. 
> `_Z8returnitIlLl4EET_v` and
> `_Z8returnitIlLi4EET_v`
>
> Am I misunderstanding something about the problem with the old mangling rules?

You need to do `returnit`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yeah, the more I think about this, the more I think that while (1) Apple should 
upstream its use of an older default, regardless (2) the existence of any 
targets at all with an older default means that tests like this always need to 
be using `-fclang-abi-compat=latest`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I've been informed that Apple's change to the default ABI compatibility mode is 
in our private fork, so this is not your problem; sorry for the noise.

I'm not sure why we decided to do that privately; I'll see if we can fix that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Hey, Richard.  It looks like your new tests are failing on Apple's buildbots, 
I'm guessing because the default ABI compatibility mode is older.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147655/new/

https://reviews.llvm.org/D147655

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37192: [clang-format] Add support for generic Obj-C categories

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Is that really the way we want to recommend formatting that, with the generics 
clause separated from the class name but not separated from the category 
clause?  I'd expect the opposite: write the generics clause with no separation 
after the class name but with separation from the category clause, e.g. 
`NSHashTable (MYFoundation)`.


Herald added a comment.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please 
try to ensure all code changes have a unit test (unless this is an `NFC` or 
refactoring, adding documentation etc..)

Add your unit tests in `clang/unittests/Format` and you can build with `ninja 
FormatTests`.  We recommend using the `verifyFormat(xxx)` format of unit tests 
rather than `EXPECT_EQ` as this will ensure you change is tolerant to random 
whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can 
happen if you are trying to improve the annotation phase to ensure we are 
correctly identifying the type of a token, please add a token annotator test in 
`TokenAnnotatorTest.cpp`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37192/new/

https://reviews.llvm.org/D37192

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> That's better than the status quo (no lifetime markers at all) but still 
> suboptimal, and I don't think it will solve the particular case I care about 
> (a particular Linux kernel driver written in C which is passing many 
> aggregates by value).

Ah, all in the same full-expression?  Then yeah, my idea wouldn't be good 
enough to optimize your situation.

> Do I want to create a flag for that? Not particularly; it does mean 
> supporting BOTH codegen patterns; I'd rather have a flag to control the 
> optimization or not do it at all rather than support 2 different 
> optimizations.

The received wisdom here is that it's very helpful to have a flag because it 
lets users test the effect of different changes independently.  Users mostly 
don't live on trunk, and LLVM releases are roughly every six months, so when 
users get a new compiler it's going to have at least six months of accumulated 
changes in it.  If a project breaks with a new compiler, it's really great to 
be able to isolate which change is causing the problem without having to 
rebuild the compiler.

The fact that we have dynamic checking of this from ASan does help a lot, 
though.

> Personally, I feel that "we found a buggy example of code we broke, we fixed 
> it. Let's reland it and tell people to be careful when upgrading." But I 
> haven't been around long enough to know what's the precedent here for 
> "aggressive optimizations." Do you feel strongly that we should not just 
> reland this, @rjmccall (or anyone else)?

That several things broke immediately after commit does suggest that there are 
quite a bit more project bugs out there waiting to blow up.  I think having a 
dedicated flag for just this optimization is probably a good idea in the short 
term.

> One thing I noticed, simply printing the expression when we were about to add 
> the lifetime cleanup; I noticed some examples would trigger on 
> CXXConstructExpr, CXXTemporaryObjectExpr, and CXXOperatorCallExpr. Those feel 
> very C++ specific; I do wonder if we could do something differently for C, 
> but I think we should not since as @rjmccall said "we'd have a consistent 
> rule for all types and targets" which IMO is nicer+simpler.

Well, if the language rules are different, they're different.  (More on that in 
a second.)  As far as I know, the lifetime of a parameter variable in C is 
never specified as outliving the call, and the entire concept of a 
full-expression is a C++ism.  We could do something more aggressive in C.

> Converting the above widget example to C, I noticed that we're also missing 
> the lifetime markers on compound literals!

Yeah, so this is probably because the lifetime of a compound literal is 
somewhat surprising in C: it's actually the containing block scope.  We could 
emit lifetime markers, but we'd have to make sure we emitted them in the right 
place.  In pure C, I think this is only detectable if the user actually takes 
the address of the literal; unlike C++, C doesn't implicitly expose the address 
of temporaries or have destructors that allow their lifetime to be directly 
tested.  But in ObjC, you could have a compound literal with weak/strong 
references in it, which arguably are required to be destroyed at the close of 
the enclosing block.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D74094#4646975 , @xbolva00 wrote:

>>> So we can start by giving these objects full-expression lifetime, chase 
>>> down any program bugs that that uncovers (which will all *unquestionably* 
>>> be program bugs under the standard), and then gradually work on landing the 
>>> more aggressive rule (perhaps even for non-trivial types).
>
> maybe provide a new flag to control this behaviour?

You mean for when we start limiting lifetimes to the call rather than the 
full-expression?  I don't know that we want such a flag in the long term, but 
having at least a -cc1 flag is usually helpful when we're bringing up 
new/stricter optimizations, yeah.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D74094#4646297 , @tellenbach wrote:

> No real comment on the issue itself but on the example as a former Eigen 
> maintainer (sorry for the noise if that's all obvious for you):
>
>   auto round (Tensor m) {
>   return (m + 0.5f).cast().cast();
>   }
>
> does not return a Tensor but an expression encoding the computation which is 
> evaluated during the assignment `const Tensor t3 = round(a.log().exp());` 
> using an overloaded `operator=`. With "evaluation" I mean evaluation the in 
> the sense of performing the intended mathematical computation.
>
> Many things can go wrong when using `auto` in such cases, of which two seem 
> to be relevant here:
>
> 1. Eigen can (this is an implementation detail(!)) decide to evaluate 
> subexpressions into temporaries. The returned expression would then reference 
> the result of such an evaluation beyond its lifetime.
> 2. If 1. does not happen, the unevaluated expression would reference its 
> arguments. The immediate `0.5f` is copied since that's cheap, but the tensor 
> would be referenced. Your example function `round` takes its parameter 
> by-value and the returned expression would reference it. I'm unsure if the 
> lifetime would be extended in this case (again, the exact details of C++ 
> lifetime rules are not my area of expertise). I think if the lifetime would 
> be extended, ASAN complaining after applying this patch is wrong, if the 
> lifetime is not extended ASAN should complain and the example is wrong.
>
> Btw, these issues are so common that Eigen documents the recommendation to 
> avoid using `auto` with Eigen types all together: 
> https://eigen.tuxfamily.org/dox/TopicPitfalls.html#title3

Okay, thanks, I can see how that works, and I definitely would never had 
figured that out from just looking at this code.  The formal lifetime of a 
parameter of type `Tensor` is definitely in the land of implementation-defined 
behavior, so this code seems to be at best non-portable.

Nick, maybe we can take a new look at this patch from that perspective.  You've 
been trying to use very tight lifetime bounds for these objects that only cover 
the duration of call, which essentially means you're defining the lifetime of 
parameter objects to just be the call rather than the full-expression (at 
least, when the parameter type doesn't have a destructor).  In the abstract, 
that's probably the right rule for Clang to adopt, because I think it matches 
programmer intuition (it's always wrong to return the address of a local, and 
that includes by-value parameters), and it means we'd have a consistent rule 
for all types and targets.  It's also a fairly aggressive rule that's likely to 
uncover a fair amount of code like this that's assuming longer lifetimes.  I 
think it's reasonable to say that these examples are all program bugs that 
should be fixed, but it might be better to pursue that as a *refinement* on top 
of an initially more conservative rule.  I suspect that that conservative rule 
will also give us a large fraction of the optimization benefit that we can 
expect from this change, because at least parameter objects from calls in 
different full-expressions will be able to share memory.  So we can start by 
giving these objects full-expression lifetime, chase down any program bugs that 
that uncovers (which will all *unquestionably* be program bugs under the 
standard), and then gradually work on landing the more aggressive rule (perhaps 
even for non-trivial types).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> Fuck me for trying to help, right? If x-values are part of the "basics" of 
> parameter passing, I'm afraid to ask about the more complicated cases.

I can see how my response was somewhat aggressive, and I regret that and 
apologize.  I imagine you're probably approaching this from the perspective of 
general optimization and are unhappy that you're getting bogged down in all 
this C++ minutiae.  It is the nature of high-level optimization, though, that 
you often have to learn a lot of language details in order to get your job 
done.  I'm happy to teach some of these concepts during review — I wouldn't 
expect more than, like, twenty people on the planet to know the differences 
between parameter and temporary lifetimes off-hand, and I understand that 
non-experts in C++ aren't going to know all these parameter initialization 
rules by heart.  But I am surprised at some of the things we've had to cover, 
like that it's not generally okay for a function to return a reference to a 
by-value parameter.  I am volunteering my time to try to help you with your 
goal of landing this patch, and it can be a little frustrating to spend that 
time on this stuff.

If you're able to build Alex's test case, I would suggest diffing the IR output 
both with and without your original patch.  -O0 output is probably fine for 
this and will be a lot easier to track back to the IRGen code responsible for 
it.  You may see something that immediately stands out as wrong, but at the 
very least, it'll tell us what's changed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D74094#4643558 , @nickdesaulniers 
wrote:

> In D74094#4643495 , @rjmccall wrote:
>
>> I can't easily tell you because the "standalone example" involves a million 
>> templates that I don't know anything about, and nobody seems to have done 
>> the analysis to identify the specific source of the miscompile.
>
> Sure; @alexfh @vitalybuka can you both please help produce a further reduced 
> test case from https://reviews.llvm.org/D74094#4633799 ? Otherwise I'm 
> beginning to think this patch may have been reverted prematurely; before an 
> understood + reduced test case was reduced.

Well, it's good to be conservative about adding new optimizations if they're 
causing apparent miscompiles.  Even if the optimization is just exposing 
existing UB in some new one, that's something we should understand as a 
project.  I have no problem with the eager revert.

It would be great if Alex and Vitaly could help track down the exact problem.  
However, if they can't, I think it's ultimately your responsibility as the 
person hoping to land the optimization to understand why the miscompile is not 
in fact your fault.

>> What I can tell you is that there is an enormous semantic difference between 
>> passing a pr-value argument to a parameter that's passed by reference and to 
>> a parameter that's passed by value.  In the former case, the reference is 
>> bound to a temporary with full-expression lifetime, and it is totally 
>> reasonable to return its address.  In the latter case, the parameter object 
>> will often have a lifetime bound by the current function, and you cannot 
>> return its address without it being UB.
>
> I'm trying to come up with an example of "the former."
>
>   struct foo {
> int x;
>   };
>   
>   // Returns a reference to the minimum foo.
>   foo (foo , foo );
>   
>   void consume (foo&);
>   
>   void bar () {
> consume(min(foo{0}, foo{1}));
>   }
>
> doesn't compile:
>
>   foo.cpp:11:11: error: no matching function for call to 'min'
>  11 |   consume(min(foo{0}, foo{1}));
> |   ^~~
>   foo.cpp:6:6: note: candidate function not viable: expects an lvalue for 1st 
> argument
>   6 | foo (foo , foo );
> |  ^   ~~
>
> Have I misinterpreted what you meant by "passing a pr-value argument to a 
> parameter that's passed by reference?"

Okay, to be frank, this is a bit of a red flag for me.  Optimizations like this 
are ultimately rooted in language rules, and I'm not sure you can effectively 
do this work if you don't understand the basics of C++ parameter passing.  In 
C++, you cannot bind a non-const l-value reference to a temporary, but you can 
bind either a const l-value reference or an x-value reference to one.  So you 
could declare `min` as either `const foo (const foo , const foo );` or 
`foo &(foo &, foo &);`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I can't easily tell you because the "standalone example" involves a million 
templates that I don't know anything about, and nobody seems to have done the 
analysis to identify the specific source of the miscompile.

What I can tell you is that there is an enormous semantic difference between 
passing a pr-value argument to a parameter that's passed by reference and to a 
parameter that's passed by value.  In the former case, the reference is bound 
to a temporary with full-expression lifetime, and it is totally reasonable to 
return its address.  In the latter case, the parameter object will often have a 
lifetime bound by the current function, and you cannot return its address 
without it being UB.

Looking closer, my previous guess that this was accidentally triggering for 
reference parameters seems to be wrong, because that case is filtered out 
earlier in the function.  I'm not sure what the problem is exactly, then, 
unless the code indeed has UB.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Under [expr.call]p6 (https://eel.is/c++draft/expr.call#6), it is 
implementation-defined whether the lifetime of a parameter ends at the end of 
the containing full expression or at the end of the function.  If the 
implementation chooses the latter, a reference to the parameter will dangle 
after the return.  In your example, on your chosen target (probably x64-64?), 
we are actually forced by the psABI to end those lifetimes at the end of the 
function, because the contents of the parameters are passed in registers 
(rather than by implicit reference) and reconstituted in the stack frame of the 
callee.  Therefore your `min` function has undefined behavior, and not just in 
a formal sense but actually in a very directly dangerous sense.  That should 
not be surprising; intuitively, you should think of this as returning a 
reference to a local variable, which is highly suspect even if it is sometimes 
— only on certain targets and for certain types — formally allowed.

> That makes me think we should probably do this lifetime annotation in the 
> caller of CodeGenFunction::EmitCallArg since it will have information about 
> the return type of the caller. If a function returns a reference to a type, 
> we probably don't want to emit the lifetime markers for any parameter of that 
> type. I wonder if strict aliasing needs to be considered, too?

This is definitely barking up the wrong tree.  The language specifies the 
lifetime of an object, and any access to the object during its lifetime is 
valid.  If the parameter's lifetime *was* guaranteed to be the full expression 
here, we would be obliged to keep it alive for that full duration.

In particular, we cannot do anything with reasoning like "the return value 
cannot be a reference to the parameter". The right way to understand that is as 
an effort to invoke the as-if rule, arguing that it is impossible to reference 
the parameter after the call if it is not part of the return value, and 
therefore we can be more aggressive than the language would formally allow. But 
it is not true that the only way to reference a parameter after a call is if it 
is part of the return value.  For example, the function could store a pointer 
to the parameter into a global variable, and as long as all accesses through 
that global happen before the parameter's lifetime ends, that's totally fine — 
it's obviously a very confusing and error-prone way to write code, but it's 
legal.  (Also, even if the address of the parameter *could* only escape into 
the return value, the type analysis would have to be much more complicated than 
just whether it's a reference to the type of a parameter.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Parameter objects are not temporaries and have their own lifetime rules, so 
there's nothing wrong with this idea in principle.  This seems to just be a 
bug, probably that we're doing a type check on `E->getType()` without 
considering whether E might be a gl-value.  We definitely do not want to be 
emitting lifetime intrinsics for the referent of a reference argument just 
because the underlying type is an aggregate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74094/new/

https://reviews.llvm.org/D74094

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158695: [clang] Fix missing contract flag in sqrt intrinsic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Code change LGTM; I'll let you hash out the test feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158695/new/

https://reviews.llvm.org/D158695

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158695: [clang] Fix missing contract flag in sqrt intrinsic

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:501
   if (CGF.Builder.getIsFPConstrained()) {
 CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E);
 Function *F = CGF.CGM.getIntrinsic(ConstrainedIntrinsicID, 
Src0->getType());

Is this existing condition not good enough, and why?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158695/new/

https://reviews.llvm.org/D158695

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Alright, LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157833/new/

https://reviews.llvm.org/D157833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158242: [Clang][Attribute] Introduce linkage attribute to specify the exact LLVM linkage type for functions or variables

[John McCall via Phabricator via cfe-commits]

rjmccall requested changes to this revision.
rjmccall added a comment.
This revision now requires changes to proceed.

I understand that you're adding this for your project's internal purposes, but 
it's a permanent language feature that I don't think we want.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158242/new/

https://reviews.llvm.org/D158242

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157379: [CodeGen] Restrict addEmittedDeferredDecl to incremental extensions

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

Thank you, this seems reasonable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157379/new/

https://reviews.llvm.org/D157379

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156537: [CodeGen] Keep track of eagerly emitted globals

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

If we're avoiding the expense here when we're not emitting incremental 
extensions, this seems fine to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156537/new/

https://reviews.llvm.org/D156537

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156897: [CodeGen] Clean up access to EmittedDeferredDecls, NFCI.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

This looks reasonable to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156897/new/

https://reviews.llvm.org/D156897

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:143
+  after leaking the coroutine handle in the await_suspend may be converted to
+  unconditional access incorrectly.
+  (`#56301 `_)

Suggestion:

```
- Fixed an issue where accesses to the local variables of a coroutine during
  ``await_suspend`` could be misoptimized, including accesses to the awaiter
  object itself.  (`#56301 
`_)
```



Comment at: clang/lib/CodeGen/CGCoroutine.cpp:159
+
+  // Return false conservatively even if the underlying type is a record type.
+  if (!Awaiter)





Comment at: clang/lib/CodeGen/CGCoroutine.cpp:169
+  // functions.
+  return !Awaiter->field_empty();
+}

Is it possible for the awaiter type to be incomplete here?  That shouldn't be 
possible if the awaiter object is returned as an r-value, but as I understand 
it, you can also return a reference to an object, which would normally allow 
the type of that object to be incomplete.  But maybe that's disallowed due to 
higher-level rules with coroutines.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:351
 
+  bool maySuspendLeakCoroutineHandle() const {
+return isCoroutine() && CurCoro.MaySuspendLeak;

ChuanqiXu wrote:
> rjmccall wrote:
> > (1) I'd prefer that we use the term "escape" over "leak" here.
> > (2) None of these bugs require us to escape the coroutine handle.
> > 
> > Maybe `mustPreventInliningOfAwaitSuspend`?  It's not really a general 
> > property.
> Used the term `escaped` instead of `leak`.
> 
> > (2) None of these bugs require us to escape the coroutine handle.
> 
> I feel like from the perspective of coroutine itself, it looks like the 
> coroutine handle escapes from the coroutine by await_suspend. So I feel this 
> may not be a bad name. Also as the above comments shows, we'd like to improve 
> this in the middle end finally, so these names would be removed in the end of 
> the day too. 
Okay, that makes sense to me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157833/new/

https://reviews.llvm.org/D157833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157452: [RFC][Clang][Codegen] `std::type_info` needs special care with explicit address spaces

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The path of least resistance here is that IRGen should just insert conversions 
from the global AS to the default AS as part of evaluating `typeid`.  I haven't 
looked at it closely, but that seems to be what this patch is doing.

However, `std::type_info` is an interesting special case in that we actually 
know statically that all objects of that type will be allocated in the global 
AS, so there's really no reason to pass around pointers in the default AS; 
`std::type_info *` should just default to being in the global AS.  It'd be a 
non-trivial feature in supported of a somewhat uncommonly-used C++ feature, and 
I can't tell how best to spend your time, *but*... if you're so inclined, I 
think it would make a somewhat compelling feature to be able to declare a 
default AS for a class type, which your target could then adopt in the C++ std 
headers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157452/new/

https://reviews.llvm.org/D157452

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:5496
+  // execution of the await_suspend. To achieve this, we need to prevent the
+  // await_suspend get inlined before CoroSplit pass.
+  //

Suggestion:

> The `await_suspend` call performed by `co_await` is essentially asynchronous
> to the execution of the coroutine.  Inlining it normally into an unsplit 
> coroutine
> can cause miscompilation because the coroutine CFG misrepresents the true
> control flow of the program: things that happen in the `await_suspend` are not
> guaranteed to happen prior to the resumption of the coroutine, and things that
> happen after the resumption of the coroutine (including its exit and the
> potential deallocation of the coroutine frame) are not guaranteed to happen
> only after the end of `await_suspend`.
>
> The short-term solution to this problem is to mark the call as uninlinable.
> But we don't want to do this if the call is known to be trivial, which is very
> common.

We don't need to get into the long-term solution here, but I'd suggest a pattern
like:

```
  call @llvm.coro.await_suspend(token %suspend, ptr %awaiter, ptr 
@awaitSuspendFn)
```

and then we just replace that with the normal call during function splitting.  
I guess we'd have to make sure we did everything right with the return values 
and exceptions, which might be a pain.  But it does have the really nice 
property that we could move all of this "is it trivial" analysis into the LLVM 
passes: if we decide that it's safe to inline the function before splitting 
(e.g. `awaitSuspendFn` doesn't contain any uses of `this`), then the 
optimization is as simple as doing the replacement in CoroEarly instead of 
after splitting.



Comment at: clang/lib/CodeGen/CGCoroutine.cpp:146
+// We can't use (CoroutineSuspendExpr).getCommonExpr()->getType() directly
+// since its type may be AutoType, ElaboratedType, ...
+class AwaiterTypeFinder : public TypeVisitor {

Please just do `getNonReferenceType()->getAsCXXRecordDecl()` and conservatively 
say it might escape if that's null.  And you really don't need to repeat the 
well-formedness checks about the awaiter type that Sema is presumably already 
going to have done; that's just asking for extra work if this code ever 
diverges from Sema, e.g. if the language standard changes.  So you should be 
able to do all of this without a visitor.



Comment at: clang/lib/CodeGen/CGCoroutine.cpp:165
+// to give users better user experience. It doesn't matter with the
+// correctness but 1 byte memory overhead.
+#ifdef NDEBUG

I'm sorry, but I'm really confused about what you're doing here.  In general, 
we really don't want compiler behavior to change based on whether we're running 
a release version of the compiler.  And this would be disabling the 
optimization completely in release builds?

Are you maybe trying to check whether we're doing an unoptimized build of the 
*program*?  That's `CodeGenOpts.OptimizationLevel == 0`.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:351
 
+  bool maySuspendLeakCoroutineHandle() const {
+return isCoroutine() && CurCoro.MaySuspendLeak;

(1) I'd prefer that we use the term "escape" over "leak" here.
(2) None of these bugs require us to escape the coroutine handle.

Maybe `mustPreventInliningOfAwaitSuspend`?  It's not really a general property.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157833/new/

https://reviews.llvm.org/D157833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156277: [Parser][ObjC] Fix parser crash on nested top-level block with better recovery path

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.  Small wording suggestion for the comment, but feel free to commit with 
that change.




Comment at: clang/lib/Parse/ParseObjc.cpp:742
 
-// Eat the identifier.
+// Bail out as if we saw an '@end'
+if (isTopLevelObjCKeyword(DirectiveKind))

"If we see something like '@interface' that's only allowed at the top level,
bail out as if we saw an '@end'.  We'll diagnose this below."


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156277/new/

https://reviews.llvm.org/D156277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

No problem, LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156539/new/

https://reviews.llvm.org/D156539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3542
+LangAS AAS = getASTAllocaAddressSpace();
+LangAS EAS = E->getType().getAddressSpace();
+if (AAS != EAS) {

`E->getType().getAddressSpace()` is actually the top-level qualification of the 
type; you need `E->getType()->getPointeeType().getAddressSpace()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156539/new/

https://reviews.llvm.org/D156539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156537: [CodeGen] Keep track of eagerly emitted globals

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I would like to avoid the overhead of this when we're not emitting for a REPL.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156537/new/

https://reviews.llvm.org/D156537

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D156539#4546834 , @AlexVlx wrote:

> In D156539#4542836 , @rjmccall 
> wrote:
>
>> We should probably write this code to work properly in case we add a target 
>> that makes `__builtin_alloca` return a pointer in the private address space. 
>>  Could you recover the target AS from the type of the expression instead of 
>> assuming `LangAS::Default`?
>
> I believe it should work as written (it is possible that I misunderstand the 
> objection, case in which I do apologise). The issue is precisely that for 
> some targets (amdgcn being one) `alloca` returns a pointer to private, which 
> doesn't compose with the default AS being unspecified / nonexistent, for some 
> languages (e.g. HIP,  C/C++ etc.). For the OCL case @arsenm mentions, I 
> believe that we make a choice based on `LangOpts.OpenCLGenericAddressSpace`, 
> and the code above would only be valid from the OCL language perspective iff 
> that is true. I've put together a short Godbolt that captures these (and the 
> bug the patch should fix, please see the bitcasts between ASes in the non-OCL 
> case): https://gcc.godbolt.org/z/sYGK76zqv.

An address space conversion is required in IR if there is a difference between 
the address space of the pointer type formally returned by the call to 
`__builtin_alloca` and the address space produced by the `alloca` operation in 
IR.  If Sema has set the type of `__builtin_alloca` to formally return 
something in the stack address space, no conversion is required.  What I'm 
saying that I'd like you to not directly refer to `LangAS::Default` in this 
code, because you're assuming that `__builtin_alloca` is always returning a 
pointer that's formally in `LangAS::Default`.  Please recover the target 
address space from the type of the expression.

Additionally, in IRGen we allow the target to hook address-space conversions; 
please call `performAddrSpaceCast` instead of directly emitting an 
`addrspacecast` instruction.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156539/new/

https://reviews.llvm.org/D156539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156277: [Parser][ObjC] Fix parser crash on nested top-level block with better recovery path

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:125
+  out early in case next top-level block occurs as if the previous one is
+  properly finished.
+  `Issue 64065 `_

"Fixed a crash when parsing top-level ObjC blocks that aren't properly 
terminated.
Clang should now also recover better when an `@end` is missing between blocks."



Comment at: clang/lib/Parse/ParseObjc.cpp:725
+case tok::objc_protocol:
+  goto MISSING_END;
+default:

This use of `goto` is a little too spaghetti-ish for me — we're breaking out of 
a loop and then branching into one case of an `if`/`else` chain.  There are two 
reasonable ways to do this with more structure.  The first is that we can make 
sure that we emit the "missing @end" diagnostic before we break out; to avoid 
code repetition, you can extract that into a helper function.  The second is 
that we could set a flag to tell the code after the loop that we need to emit 
that diagnostic instead of trying to recreate the case analysis that we did in 
the loop.

If you make a little helper function like `isTopLevelObjCKeyword` then you can 
just `break` out instead of having the awkwardness of being inside a `switch`.



Comment at: clang/lib/Parse/ParseObjc.cpp:737
   break;
 } else if (DirectiveKind == tok::objc_not_keyword) {
   Diag(Tok, diag::err_objc_unknown_at);

I would suggest pulling this up to right after (or even before) the 
`isTopLevelObjCKeyword` check.  Then after the check you know that you're 
dealing with a keyword that you actually want to handle, and you  can consume 
both the `@` and the keyword token in one place.



Comment at: clang/lib/Parse/ParseObjc.cpp:827
   // EOF.  In the former case, eat the @end.  In the later case, emit an error.
   if (Tok.is(tok::code_completion)) {
 cutOffParsing();

I don't think this case is actually reachable — we handle code-completion 
within the loop and then return.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156277/new/

https://reviews.llvm.org/D156277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We should probably write this code to work properly in case we add a target 
that makes `__builtin_alloca` return a pointer in the private address space.  
Could you recover the target AS from the type of the expression instead of 
assuming `LangAS::Default`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156539/new/

https://reviews.llvm.org/D156539

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156277: [Parser][ObjC] Stop parsing on eof

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Parse/ParseObjc.cpp:749
+  if (!Tok.is(tok::eof))
+ConsumeToken();
   break;

danix800 wrote:
> rjmccall wrote:
> > aaron.ballman wrote:
> > > rjmccall wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > danix800 wrote:
> > > > > > > tbaeder wrote:
> > > > > > > > Why is there a `ConsumeToken()` call at all here? The token is 
> > > > > > > > already being consumed in line 729.
> > > > > > > Didn't notice this, thanks for reminding!
> > > > > > I have the same question as @tbaeder -- what token is this 
> > > > > > intending to consume? CC @rjmccall for Obj-C expertise
> > > > > OH! This is consuming the identifier for the implementation/interface 
> > > > > name itself. e.g.,
> > > > > ```
> > > > > @interface Frobble
> > > > > ```
> > > > > The consume on line 709 gets the `@`, the consume on line 729 gets 
> > > > > the `interface`, and the consume on line 749 is getting the 
> > > > > `Frobble`. That makes sense to me now.
> > > > > 
> > > > I don't think any language expertise is required here — just seems like 
> > > > a straightforward bug on an error path that's probably not exercised 
> > > > all that often.  Maybe somebody moved the `ConsumeToken` and forgot to 
> > > > fix this case or something.
> > > What concerns me about this fix is that we don't typically check whether 
> > > the token is EOF or not before consuming; that's usually an anti-pattern, 
> > > isn't it? Wouldn't it make sense for this to use 
> > > `SkipUntil(tok::identifier)` instead?
> > Okay, so now I can bring a little language expertise to bear. :)
> > 
> > We're in the middle of parsing an ObjC block (e.g. `@interface`), and we 
> > see `@interface` or `@implementation`, which starts a new block.  You can 
> > never nest these ObjC blocks, so the parser is reasonably assuming that the 
> > second `@keyword` is an attempt to start a new block and the user just 
> > forgot to terminate the last block with `@end`.  Unfortunately, the actual 
> > recovery done by the parser doesn't seem to match the diagnostic and the 
> > fixit — it's trying to swallow `@interface Foo` (or whatever) and then 
> > continue the loop as if it were part of the current block, which is 
> > definitely not the right thing to do.
> > 
> > The right way to recover here is to act like we actually saw `@end` and 
> > break out of the loop, leaving `Tok` on the `@` so that the parser will 
> > pick up parsing `@interface` normally after we return.  To do that, we just 
> > need to get the ObjC keyword by peeking at the next token instead of 
> > consuming.
> > 
> > Also, we should take this recovery path on every `@` keyword that's only 
> > allowed at the top level (so `@class`, `@compatibility_alias`, 
> > `@interface`, `@implementation`, and `@protocol`).
> It's really great to learn things here! I don't know two much about ObjC. I 
> seached google trying to find some standard or specs for ObjC but only docs 
> like tutorials teaching how to use it can be found, so I might not be able to 
> give a good enough fix for this issue. I'll give it a try though.
You should be able to follow the guidance here without needing to know much 
more about ObjC, just understanding how the parser works.  The key is that you 
need to delay consuming tokens until you're certain you're going to commit to 
parsing this `@`-directive as part of the current declaration.

Start with the line `SourceLocation AtLoc = ConsumeToken();`  Instead of 
consuming the `@` and then looking at `Tok` to see what the keyword is, you can 
get the location of `Tok` without consuming it, then use `NextToken()` to peek 
ahead to the next token to see the keyword.

Be sure to sink the right number of `ConsumeToken` calls down onto all of the 
paths that *aren't* bailing out.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156277/new/

https://reviews.llvm.org/D156277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156277: [Parser][ObjC] Stop parsing on eof

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Parse/ParseObjc.cpp:749
+  if (!Tok.is(tok::eof))
+ConsumeToken();
   break;

aaron.ballman wrote:
> rjmccall wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > danix800 wrote:
> > > > > tbaeder wrote:
> > > > > > Why is there a `ConsumeToken()` call at all here? The token is 
> > > > > > already being consumed in line 729.
> > > > > Didn't notice this, thanks for reminding!
> > > > I have the same question as @tbaeder -- what token is this intending to 
> > > > consume? CC @rjmccall for Obj-C expertise
> > > OH! This is consuming the identifier for the implementation/interface 
> > > name itself. e.g.,
> > > ```
> > > @interface Frobble
> > > ```
> > > The consume on line 709 gets the `@`, the consume on line 729 gets the 
> > > `interface`, and the consume on line 749 is getting the `Frobble`. That 
> > > makes sense to me now.
> > > 
> > I don't think any language expertise is required here — just seems like a 
> > straightforward bug on an error path that's probably not exercised all that 
> > often.  Maybe somebody moved the `ConsumeToken` and forgot to fix this case 
> > or something.
> What concerns me about this fix is that we don't typically check whether the 
> token is EOF or not before consuming; that's usually an anti-pattern, isn't 
> it? Wouldn't it make sense for this to use `SkipUntil(tok::identifier)` 
> instead?
Okay, so now I can bring a little language expertise to bear. :)

We're in the middle of parsing an ObjC block (e.g. `@interface`), and we see 
`@interface` or `@implementation`, which starts a new block.  You can never 
nest these ObjC blocks, so the parser is reasonably assuming that the second 
`@keyword` is an attempt to start a new block and the user just forgot to 
terminate the last block with `@end`.  Unfortunately, the actual recovery done 
by the parser doesn't seem to match the diagnostic and the fixit — it's trying 
to swallow `@interface Foo` (or whatever) and then continue the loop as if it 
were part of the current block, which is definitely not the right thing to do.

The right way to recover here is to act like we actually saw `@end` and break 
out of the loop, leaving `Tok` on the `@` so that the parser will pick up 
parsing `@interface` normally after we return.  To do that, we just need to get 
the ObjC keyword by peeking at the next token instead of consuming.

Also, we should take this recovery path on every `@` keyword that's only 
allowed at the top level (so `@class`, `@compatibility_alias`, `@interface`, 
`@implementation`, and `@protocol`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156277/new/

https://reviews.llvm.org/D156277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156277: [Parser][ObjC] Stop parsing on eof

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Parse/ParseObjc.cpp:749
+  if (!Tok.is(tok::eof))
+ConsumeToken();
   break;

aaron.ballman wrote:
> danix800 wrote:
> > tbaeder wrote:
> > > Why is there a `ConsumeToken()` call at all here? The token is already 
> > > being consumed in line 729.
> > Didn't notice this, thanks for reminding!
> I have the same question as @tbaeder -- what token is this intending to 
> consume? CC @rjmccall for Obj-C expertise
I don't think any language expertise is required here — just seems like a 
straightforward bug on an error path that's probably not exercised all that 
often.  Maybe somebody moved the `ConsumeToken` and forgot to fix this case or 
something.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156277/new/

https://reviews.llvm.org/D156277

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154774: [Sema] Respect instantiated-from template's VisibilityAttr for two implicit/explicit instantiation cases

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/AST/Decl.cpp:380
+->getTemplatedDecl()
+->hasAttr();
 }

Okay, this change seems wrong.  A visibility attribute on an explicit 
specialization or instantiation should definitely override everything else.  A 
visibility attribute on a template pattern should only override other 
visibility information that we would derive from that pattern, like the 
visibility of the template parameters; it should not override visibility from 
the template arguments.  You probably need this function to return an enum with 
three cases: (1) factor in both template arguments and template parameters, (2) 
factor in only template arguments, and (3) factor in nothing.

Also, the bug here doesn't seem to be specific to function templates, and you 
should fix the code for all three paths (function templates, class templates, 
and variable templates).  Basically we're universally mishandling visibility 
attributes on template patterns for templates with non-type template parameters 
with hidden visibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154774/new/

https://reviews.llvm.org/D154774

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155396: [Sema][ObjC] Invalidate BlockDecl with invalid return expr & its parent BlockExpr

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Dropping the entire block from the AST on error would be unfortunate for some 
kinds of tooling, but as long as we maintain it with a `RecoveryExpr`, that 
seems like a fine approach to me.

As a general rule, I think we should be tracking basically everything on the 
`BlockDecl` and let `BlockExpr` just have a reference to that on top of the 
baseline `Expr` storage.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155396/new/

https://reviews.llvm.org/D155396

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Okay, LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146242/new/

https://reviews.llvm.org/D146242

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thank you, this looks great, and I really appreciate that you found a way to 
make it work with just the single loop (in the typical case).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154503/new/

https://reviews.llvm.org/D154503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGVTables.cpp:693
 return Int32Ty;
-  return Int8PtrTy;
+  return GlobalsInt8PtrTy;
 }

bjope wrote:
> I noticed that we have some old fixes downstream that conflicts with the 
> changes you've made here. I thought that perhaps we could get rid of those 
> now when you've fixed the code upstream.
> 
> Isn't the VTable holding function pointers when not using the relative 
> layout, and then this should be a pointer to the ProgramAddressSpace and not 
> a pointer to the DefaultGlobalsAddressSpace?
> 
> Downstream we've been using a special `FnVoidPtrTy` here. Defined as 
> `FnVoidPtrTy = Int8Ty->getPointerTo(DL.getProgramAddressSpace());`.
> 
It's a mix.  The `type_info` pointer should be in the global address space 
(although it would be forgivable to just use the default address space), the 
top, vbase, and vcall offsets are all `ptrdiff_t`s (presumably the same size as 
the default address space), and the virtual functions are function pointers.  
If we're going to support a target where those can be different sizes, we 
probably need to start computing a byte layout of the v-table and doing byte 
GEPs into it, because our current IR patterns are naively assuming the 
components are all the same size.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153092/new/

https://reviews.llvm.org/D153092

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

LGTM, except, should we have a way to turn this optimization off specifically?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155759: [Clang][CodeGen] Follow-up for `vtable`, `typeinfo` et al. are globals

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155759/new/

https://reviews.llvm.org/D155759

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I'm suggesting:

  llvm::BitVector declsToRemove;
  
  while (I < N) {
if (shouldHideTag()) declsToRemove.add(I);
if (notUnique()) declsToRemove.add(I);
  }
  
  Decls.removeAll(declsToRemove)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154503/new/

https://reviews.llvm.org/D154503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yes, this diagnostic implements a core semantic rule and must be emitted.  
There are some situations where Clang will actually generate malformed IR 
(violating dominance rules) if you fail to diagnose jump violations correctly.  
The IR you get when there's a jump over a destructor is well-formed, but it's 
still arguably* a miscompile and must be diagnosed.

(*) It's a miscompile unless there's a rule saying it's U.B. to jump out of a 
scope.  I don't see such a rule in any of the docs about this feature.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> We need to check the scopes like we would for direct goto, because we don't 
> want to bypass non-trivial destructors.

I think this is the basic misunderstanding here.  Direct `goto` is allowed to 
jump out of scopes; it just runs destructors on the way.  It is only a direct 
branch to the target block if there are no destructors along the path.

Indirect `goto` is not allowed to jump out of scopes because, in general, the 
labels could be in different scopes with different sets of destructors 
required, and so the only way we could run the right set of destructors along 
the way would be to dynamically compare the label value with specific labels, 
which would defeat the point of the feature.  (This is unnecessary if all the 
labels are in the same scope, which is typical, but nobody has felt like 
extending the feature to recognize that pattern and be more general.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

nickdesaulniers wrote:
> rjmccall wrote:
> > I think it would be good to leave a comment here like this:
> > 
> > > We know the possible destinations of an asm goto, but we don't have the
> > > ability to add code along those control flow edges, so we have to diagnose
> > > jumps both in and out of scopes, just like we do with an indirect goto.
> Depending on your definition of "we" (clang vs. llvm), llvm does have the 
> ability to add code along those edges.
> 
> See llvm/lib/CodeGen/CallBrPrepare.cpp.  I'll clarify in your comment that 
> "clang does not split critical edges during code generation of llvm ... "
Okay, so, I don't really know much about this feature.  I was thinking that the 
branch might go directly into some other assembly block, which would not be 
splittable.  If the branch just goes to an arbitrary basic block in IR, then it 
would be straightforward for IRGen to just resolve the destination blocks for 
`asm goto` labels to some new block that does a normal `goto` to that label.  
If we did that, we wouldn't need extra restrictions here at all and could just 
check this like any other direct branch.

We don't need to do that work right away, but the comment should probably 
reflect the full state of affairs — "but clang's IR generation does not 
currently know how to add code along these control flow edges, so we have to 
diagnose jumps both in and out of scopes, like we do with indirect goto.  If we 
ever add that ability to IRGen, this code could check these jumps just like 
ordinary `goto`s."


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

I think it would be good to leave a comment here like this:

> We know the possible destinations of an asm goto, but we don't have the
> ability to add code along those control flow edges, so we have to diagnose
> jumps both in and out of scopes, just like we do with an indirect goto.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155506: [clang][JumpDiagnostics] use StmtClass rather than dyn_cast chain NFC

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D155506#4508176 , @gribozavr2 
wrote:

> I'm not sure it is actually an anti-pattern. `dyn_cast` will transparently 
> handle subclasses, should any be added in the future, but a switch wouldn't.

Yeah, I don't think we have a strong style preference either way, here, given 
that this can't be an exhaustive switch anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155506/new/

https://reviews.llvm.org/D155506

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155525: WIP

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:362
 // it.  This makes the second scan not have to walk the AST again.
+RecordJumpScope:
 LabelAndGotoScopes[S] = ParentScope;

I would indent this at the same level as the `case` labels, and group it with 
them, too, but yeah, this is what I was thinking.  But the helper function 
approach looks pretty clean, too.  Your choice.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155525/new/

https://reviews.llvm.org/D155525

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I do like the look of https://reviews.llvm.org/D155522, yeah, since we have so 
many of these that aren't top-level in a case anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

nickdesaulniers wrote:
> rjmccall wrote:
> > You can pull the `GCCAsmStmtClass` case right above this, make the cast 
> > unconditional (`if (!cast(S)->isAsmGoto()) break;`), and then 
> > fall through into the GotoStmt case.
> I could hoist + use `[[fallthrough]]` but note that the case above 
> `Stmt::SwitchStmtClass` also currently uses `[[fallthrough]]` here as well; 
> so I would still need the `dyn_cast`.
> 
> With that in mind, do you still prefer the hoisting? I don't have a 
> preference, but wanted to triple check that with you.
Ah, right.  Personally, I would probably put this common path in a helper 
function, but you could also just duplicate it since it's so small.  This also 
seems like an acceptable use-case for `goto` if you can stomach that — with a 
good label, it should still be very readable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, that's way better.  Just a couple minor requests.




Comment at: clang/docs/ReleaseNotes.rst:629
+- Fixed false positive error diagnostic observed from mixing ``asm goto`` with
+  ``__attribute__((cleanup()))`` variables falsly warning that jumps to
+  non-targets would skip cleanup.





Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

You can pull the `GCCAsmStmtClass` case right above this, make the cast 
unconditional (`if (!cast(S)->isAsmGoto()) break;`), and then fall 
through into the GotoStmt case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Wait, the whole point of this algorithm is that we have to do this whole 
complicated linear check against every label whose address is taken because we 
don't know where it's going.  If we have a list of all the possible labels that 
the `asm goto` might jump to, why are we building a map of all the labels and 
then filtering out the ones that aren't listed?  We should just be checking the 
listed destinations with the stricter triviality rule that indirect-goto uses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D154658#4482202 , @rsmith wrote:

> In D154658#4481225 , @rjmccall 
> wrote:
>
>> In D154658#4479213 , @rsmith wrote:
>>
>>> I think (hope?) we should be able to apply this to a much larger set of 
>>> cases. Would it be correct to do this optimization unless the vtable might 
>>> be emitted with vague linkage and non-default visibility (that is, unless 
>>> we're in the odd case where people expect non-default visibility classes to 
>>> be the same type across DSOs)? Or are there cases where we might be using a 
>>> vtable that (eg) doesn't even have the right symbol?
>>
>> I don't know of any problems other than the total failure of vague linkage 
>> across DSO boundaries, so if we just treat that as an implicit exception to 
>> the vtable uniqueness guarantee in the ABI, I think you've got the condition 
>> exactly right: we could do this for any class where either the v-table 
>> doesn't have vague linkage or the class's formal visibility is not 
>> `default`.  Our experience at Apple with enforcing type visibility is that 
>> it's usually good for one or two bug reports a year, but it's pretty easy to 
>> explain to users what they did wrong, and Clang's visibility attributes are 
>> pretty simple to use.  (`type_visibility` helps a lot with managing 
>> tradeoffs.)
>
> I did some checking through the Clang implementation and found another two 
> cases:
>
> - under `-fapple-kext`, vague-linkage vtables get emitted in each translation 
> unit that references them
> - under `-fno-rtti`, identical vtables for distinct types could get merged 
> because we emit vtables as `unnamed_addr` (this doesn't affect 
> `dynamic_cast`, because `-fno-rtti` also disables `dynamic_cast` entirely)
>
> The good news seems to be that if you don't use any language extensions (type 
> visibility, `-fno-rtti`, `-fapple-kext`), then we do actually provide the 
> guarantee that the ABI describes. :)
>
> In D154658#4479170 , @rjmccall 
> wrote:
>
>> If there are multiple subobjects of the source type in the destination type, 
>> consider just casting to `void*` first instead of doing multiple comparisons.
>
> This turned out to be a little subtle: the vptr comparison we do after the 
> cast requires loading a vptr of an object of entirely-unknown type. This is 
> the first time we're doing that in IR generation, and `GetVTablePtr` expected 
> to be told which class it's loading the vtable for (presumably, so that it 
> can load from the right offset within the class). However, the implementation 
> of `GetVTablePtr` didn't use the class for anything; it's always at the start 
> for all of our supported ABIs. But this patch would make it harder to support 
> an ABI that didn't put the vptr at the start of the most-derived object.

Oh, this does matter on platforms using pointer authentication, because each 
vptr field is signed with a constant discriminator derived from the name of the 
class that introduced it.




Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:204
+// another type.
+if (!CGM.GetAddrOfRTTIDescriptor(RecordTy))
+  return false;

Is there no reasonable way of checking this without actually triggering the 
emission of RTTI?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:603
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+// Whether the PseudoObjectExpr has result.
+unsigned HasResult : 1;

aaron.ballman wrote:
> 
Please remove the comment, which is incorrect.  Otherwise, I think this is fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154784/new/

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaLookup.cpp:542
+N = Decls.size();
+  }
+

dexonsmith wrote:
> john.brawn wrote:
> > rjmccall wrote:
> > > john.brawn wrote:
> > > > rjmccall wrote:
> > > > > This is going to fire on every single ordinary lookup that finds 
> > > > > multiple declarations, right?  I haven't fully internalized the issue 
> > > > > you're solving here, but this is a very performance-sensitive path in 
> > > > > the compiler; there's a reason this code is written to very carefully 
> > > > > only do extra work when we've detected in the loop below that we're 
> > > > > in a hidden-declarations situation.  Is there any way we can restore 
> > > > > that basic structure?
> > > > Test4 in the added tests is an example of why we can't wait until after 
> > > > the main loop. The `using A::X` in namespace D is eliminated by the 
> > > > UniqueResult check, so the check for a declaration being hidden can 
> > > > only see the using declarations in namespace C. We also can't do it in 
> > > > the loop itself, as then we can't handle Test5: at the time we process 
> > > > the `using A::X` in namespace D it looks like it may cause ambiguity, 
> > > > but it's later hidden by the `using B::X` in the same namespace which 
> > > > we haven't yet processed.
> > > > 
> > > > I have adjusted it though so the nested loop and erasing of decls only 
> > > > happens when we both have things that hide and things that can be 
> > > > hidden. Doing some quick testing of compiling SemaOpenMP.cpp (the 
> > > > largest file in clang), LookupResult::resolveKind is called 75318 
> > > > times, of which 75283 calls have HideTags=true, of which 56 meet this 
> > > > condition, i.e. 0.07%.
> > > Okay, I can see why you need to not mix tag-hiding with the removal of 
> > > duplicates.  However, I think you can maintain the current structure by 
> > > delaying the actual removal of declarations until after the main loop; 
> > > have the loop build up a set of indices to remove instead.  (Also, you 
> > > can keep this set as a bitset instead of a `std::set`.)
> > > 
> > > It's a shame that the hiding algorithm has to check every other 
> > > declaration in the lookup in case they're from different scopes.  I guess 
> > > to avoid that we'd have to do the filtering immediately when we collect 
> > > the declarations from a particular DC.
> > I think that delaying the removal until after the main loop would just 
> > complicate things, as then in the main loop we would have to check each 
> > index to see if it's something we're going to later remove. I can adjust it 
> > to do the erasing more like it's done in the main loop though, i.e. move 
> > the erased element to the end and decrement N, so the call to 
> > Decls.truncate will remove it. We can't use bitset though, as that takes 
> > the size of the bitset (which in this case would be the number of decls) as 
> > a template parameter.
> llvm::BitVector should work for this. 
Why would the main loop need to check indices to see if it's something we're 
going to remove?  You just need to check whether a tag is hidden before you add 
it to `UniqueTypes`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154503/new/

https://reviews.llvm.org/D154503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We had that discussion in the bug report.  Let's just increase the widths 
unconditionally; this is not such a common expression class that we need to 
worry about using an extra word.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154784/new/

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153835: [Sema] Clone VisibilityAttr for functions in template instantiations

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I agree that the change to test51 is the right result, yes.  The explicit 
visibility attribute on the template declaration should take precedence over 
the visibility of the types in the template parameter list, and then the 
explicit visibility attribute on the variables should take precedence over the 
visibility of the variable types, and then the instantiation should be a 
specialization of a default-visibility template with default-visibility 
arguments.  But I think that logic ought to be fixed in `getLVFor...`, not by 
changing the basic propagation of attributes.

> I actually think that the patch makes VisibilityAttr behave in a more normal 
> way as the majority of attributes are cloned by instantiateTemplateAttribute 
> in the tablegen-generated 
> tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc.

You're not really just cloning it, because making the cloned attribute implicit 
(i.e. giving it the effect of a visibility pragma instead of an attribute) is a 
huge difference.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153835/new/

https://reviews.llvm.org/D153835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaLookup.cpp:542
+N = Decls.size();
+  }
+

john.brawn wrote:
> rjmccall wrote:
> > This is going to fire on every single ordinary lookup that finds multiple 
> > declarations, right?  I haven't fully internalized the issue you're solving 
> > here, but this is a very performance-sensitive path in the compiler; 
> > there's a reason this code is written to very carefully only do extra work 
> > when we've detected in the loop below that we're in a hidden-declarations 
> > situation.  Is there any way we can restore that basic structure?
> Test4 in the added tests is an example of why we can't wait until after the 
> main loop. The `using A::X` in namespace D is eliminated by the UniqueResult 
> check, so the check for a declaration being hidden can only see the using 
> declarations in namespace C. We also can't do it in the loop itself, as then 
> we can't handle Test5: at the time we process the `using A::X` in namespace D 
> it looks like it may cause ambiguity, but it's later hidden by the `using 
> B::X` in the same namespace which we haven't yet processed.
> 
> I have adjusted it though so the nested loop and erasing of decls only 
> happens when we both have things that hide and things that can be hidden. 
> Doing some quick testing of compiling SemaOpenMP.cpp (the largest file in 
> clang), LookupResult::resolveKind is called 75318 times, of which 75283 calls 
> have HideTags=true, of which 56 meet this condition, i.e. 0.07%.
Okay, I can see why you need to not mix tag-hiding with the removal of 
duplicates.  However, I think you can maintain the current structure by 
delaying the actual removal of declarations until after the main loop; have the 
loop build up a set of indices to remove instead.  (Also, you can keep this set 
as a bitset instead of a `std::set`.)

It's a shame that the hiding algorithm has to check every other declaration in 
the lookup in case they're from different scopes.  I guess to avoid that we'd 
have to do the filtering immediately when we collect the declarations from a 
particular DC.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154503/new/

https://reviews.llvm.org/D154503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153835: [Sema] Clone VisibilityAttr for functions in template instantiations

[John McCall via Phabricator via cfe-commits]

rjmccall requested changes to this revision.
rjmccall added a comment.
This revision now requires changes to proceed.

We made a decision ten years or so ago to use a slightly different 
interpretation of the visibility attributes, so differences from GCC are not by 
themselves unexpected or problematic.  In this case, I'm not sure I agree that 
it's a bug that an attribute on an explicit class template instantiation 
overrides attributes on member functions in the template pattern.  Even if we 
decide to adopt a new rule here, though, I agree with Eli that this seems like 
a very heavy-handed way of implementing it; basically every line in this patch 
seems to be impactful in ways that are hard to anticipate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153835/new/

https://reviews.llvm.org/D153835

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139629: clang: Stop emitting "strictfp"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D139629#4481030 , @zahiraam wrote:

> According to this comment https://reviews.llvm.org/D87528#2342132, it looks 
> like @rjmccall had proposed the addition of the attribute. @rjmccall Do you 
> recall why it was added?

My suggestion was that we track the pragma in the Clang AST with an attribute 
instead of a bit on the Decl.  I have no idea why IR generation is adding both 
a string attribute and the builtin attribute to the IR function, but if doesn't 
have any obvious effect, I suggest just removing it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139629/new/

https://reviews.llvm.org/D139629

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D154658#4479213 , @rsmith wrote:

> In D154658#4479170 , @rjmccall 
> wrote:
>
>> I don't think it's an intended guarantee of the Itanium ABI that the v-table 
>> will be unique, and v-tables are frequently not unique in the presence of 
>> shared libraries.
>
> https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vtable-general explicitly 
> guarantees this: "[...] However, the virtual table pointers within all the 
> objects (instances) of a particular most-derived class point to the same set 
> of virtual tables."

Huh, I've never noticed that before.  I still don't think it's satisfied in 
practice, at least in the presence of shared libraries.  This is notably the 
exact same setup that eventually forced (or "forced") GCC to start using string 
comparison for RTTI instead of the ABI-prescribed algorithm that relies on 
`_ZTS` pointer equality.

>> They should be unique for classes with internal linkage, but of course 
>> that's a vastly reduced domain for the optimization.
>
> I think (hope?) we should be able to apply this to a much larger set of 
> cases. Would it be correct to do this optimization unless the vtable might be 
> emitted with vague linkage and non-default visibility (that is, unless we're 
> in the odd case where people expect non-default visibility classes to be the 
> same type across DSOs)? Or are there cases where we might be using a vtable 
> that (eg) doesn't even have the right symbol?

I don't know of any problems other than the total failure of vague linkage 
across DSO boundaries, so if we just treat that as an implicit exception to the 
vtable uniqueness guarantee in the ABI, I think you've got the condition 
exactly right: we could do this for any class where either the v-table doesn't 
have vague linkage or the class's formal visibility is not `default`.  Our 
experience at Apple with enforcing type visibility is that it's usually good 
for one or two bug reports a year, but it's pretty easy to explain to users 
what they did wrong, and Clang's visibility attributes are pretty simple to 
use.  (`type_visibility` helps a lot with managing tradeoffs.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I don't think it's an intended guarantee of the Itanium ABI that the v-table 
will be unique, and v-tables are frequently not unique in the presence of 
shared libraries.  They should be unique for classes with internal linkage, but 
of course that's a vastly reduced domain for the optimization.  We could do an 
RTTI comparison in the general case, either inline or by synthesizing a helper 
function to compare two `std::type_info`s.  Or you could have a flag to tell 
you to make stronger uniqueness assumptions.

If there are multiple subobjects of the source type in the destination type, 
consider just casting to `void*` first instead of doing multiple comparisons.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154658/new/

https://reviews.llvm.org/D154658

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaLookup.cpp:542
+N = Decls.size();
+  }
+

This is going to fire on every single ordinary lookup that finds multiple 
declarations, right?  I haven't fully internalized the issue you're solving 
here, but this is a very performance-sensitive path in the compiler; there's a 
reason this code is written to very carefully only do extra work when we've 
detected in the loop below that we're in a hidden-declarations situation.  Is 
there any way we can restore that basic structure?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154503/new/

https://reviews.llvm.org/D154503

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86993: Document Clang's expectations of the C standard library.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

You can't just consider the declared function signatures; `memcpy`ing two 
non-empty objects with perfect overlap is a violation of `restrict`, and yet 
IIRC GCC is known to assume that that works in some places.  (I can't duplicate 
that off-hand, though — GCC seems determined to generate inlined `memcpy`s in 
my quick efforts to test this.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86993/new/

https://reviews.llvm.org/D86993

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86993: Document Clang's expectations of the C standard library.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D86993#4474267 , @RalfJung wrote:

> The first point is important for LLVM's own memcpy/memmove intrinsics, which 
> are documented as NOPs on size 0 (and e.g. Rust relies on that).

Right, I understand that these assumptions come directly from the stronger 
semantics offered by the LLVM intrinsics.  The C committee is not going to find 
that compelling, though — we don't get to default-win arguments just because 
we've defined an IR with stronger semantics than necessary.  They are going to 
want to see arguments about why it's valuable for the C library to have these 
stronger semantics, which for us means talking about code patterns in user 
programs that take advantage of those stronger semantics and the benefits they 
see from that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86993/new/

https://reviews.llvm.org/D86993

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86993: Document Clang's expectations of the C standard library.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I agree with Aaron that it would be better to change the C standard if we can.  
I don't know how important the first bullet is; IIRC it enables some useful 
middle-end transformation.  I know the second is useful in the frontend so that 
we don't have to do explicit pointer equality checks around aggregate 
assignments, although in many cases we'd be able to avoid that because e.g. we 
know that one of the operands is a temporary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86993/new/

https://reviews.llvm.org/D86993

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153590: Don't use float_t and double_t with #pragma clang fp eval_method.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153590/new/

https://reviews.llvm.org/D153590

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D153590: Don't use float_t and double_t with #pragma clang fp eval_method.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

`abi-check` is a really generic name for a test case; could you rename those 
three tests to something more specific to this feature?  Also, your tests are 
only testing this behavior in C++.

Patch functionality LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153590/new/

https://reviews.llvm.org/D153590

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152818: [Clang] Make template instantiations respect pragma FENV_ACCESS state at point-of-definition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Does https://reviews.llvm.org/D143241 solve the original problem here, or is 
there something deeper?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152818/new/

https://reviews.llvm.org/D152818

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143241: [Clang] Reset FP options before function instantiations

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Hmm.  Why are we clearing the FP pragma stack instead of saving the old context 
onto it and then restoring after instantiation?  I don't think semantic 
analysis ever depends on enclosing members of the stack, does it?

Clearing the entire stack might not matter much if we're at the end of the 
translation unit, which is the normal time to instantiate things, but it would 
matter if we're eagerly instantiating within the translation unit, which we 
have to do for various reasons, including explicit instantiation and 
`constexpr`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143241/new/

https://reviews.llvm.org/D143241

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: [clang] Add a namespace for interesting identifiers.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: [clang] Add a namespace for interesting identifiers.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yes, this is very close, thank you.




Comment at: clang/include/clang/Basic/TokenKinds.def:805
+INTERESTING_IDENTIFIER(float_t)
+INTERESTING_IDENTIFIER(double_t)
+INTERESTING_IDENTIFIER(FILE)

I think it would be cleaner if you added these two as interesting identifiers 
in your follow-up patch.



Comment at: clang/lib/Sema/SemaLookup.cpp:950
+  return true;
+}
 // In C++ and OpenCL (spec v1.2 s6.9.f), we don't have any predefined

The two changes in this file shouldn't be necessary, since interesting 
identifiers should be disjoint from builtins.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143241: [Clang] Reset FP options before function instantiations

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Parse/ParseTemplate.cpp:1736
+  // point of the template definition.
+  Actions.resetFPOptions(LPT.FPO);
+

Ah, is this bug specific to the MSVC-compatible template logic?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143241/new/

https://reviews.llvm.org/D143241

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:6784
+  if (II->getInterestingIdentifierID() != 0)
+NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context));
 }

zahiraam wrote:
> rjmccall wrote:
> > Please switch over the interesting identifiers here; we don't want to 
> > assume this feature is only used for these two names.
> > 
> > In fact, should we go ahead and immediately apply it to the four 
> > identifiers above this?  That would be nice, because then we could actually 
> > do this in two patches: one patch that does the refactor to track 
> > interesting identifiers but doesn't cause any functionality changes and a 
> > second, very small patch that just introduces the new special treatment for 
> > `float_t` and `double_t`.
> > Please switch over the interesting identifiers here; we don't want to 
> > assume this feature is only used for these two names.
> > 
> > In fact, should we go ahead and immediately apply it to the four 
> > identifiers above this?  That would be nice, because then we could actually 
> > do this in two patches: one patch that does the refactor to track 
> > interesting identifiers but doesn't cause any functionality changes and a 
> > second, very small patch that just introduces the new special treatment for 
> > `float_t` and `double_t`.
> 
> Are you saying that "FILE", "jmp_buf"," sigjmp_buf" and "ucontext_t" are also 
> interesting identifiers? If yes, they should be added to the list of 
> interesting identifiers in TokenKinds.def?
Right.  The basic idea of interesting identifiers is to replace these sorts of 
identifier comparisons in performance-critical code.  So your first patch would 
*only* add those four identifiers as interesting identifiers, handling them 
here by registering the `typedef` with the ASTContext like the code is already 
doing.  Then you'd make a follow-up patch that adds `float_t` and `double_t` 
and handles them here by implicitly adding your new attribute.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:6784
+  if (II->getInterestingIdentifierID() != 0)
+NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context));
 }

Please switch over the interesting identifiers here; we don't want to assume 
this feature is only used for these two names.

In fact, should we go ahead and immediately apply it to the four identifiers 
above this?  That would be nice, because then we could actually do this in two 
patches: one patch that does the refactor to track interesting identifiers but 
doesn't cause any functionality changes and a second, very small patch that 
just introduces the new special treatment for `float_t` and `double_t`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/IdentifierTable.h:341
+  void setInterestingIdentifierID(unsigned ID) {
+assert(ID != FirstBuiltinID);
+ObjCOrBuiltinID = FirstInterestingIdentifierID + (ID - 1);

Similarly, this assertion needs to be `!= tok::not_interesting` (i.e. `!= 0`).



Comment at: clang/include/clang/Basic/IdentifierTable.h:326
+ObjCOrBuiltinID = FirstBuiltinID + (ID - 1);
+assert(getBuiltinID() == ID && "ID too large for field!");
+  }

zahiraam wrote:
> Is this the assert you are looking for?
No, I want you to assert that `ID` (the argument) is not 0, because if it is 
you'll end up setting `ObjCOrBuiltinID` to `FirstBuiltinID - 1`, i.e. 
`LastInterestingIdentifierID`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/IdentifierTable.h:325
   void setBuiltinID(unsigned ID) {
-ObjCOrBuiltinID = ID + tok::NUM_OBJC_KEYWORDS;
-assert(ObjCOrBuiltinID - unsigned(tok::NUM_OBJC_KEYWORDS) == ID
-   && "ID too large for field!");
+ObjCOrBuiltinID = FirstBuiltinID + (ID - 1);
+assert(getBuiltinID() == ID && "ID too large for field!");

zahiraam wrote:
> rjmccall wrote:
> > `initializeBuiltins` does call `setBuiltinID(NotBuiltin)` in order to reset 
> > any identifiers for which we have a  `-fno-builtin=X` argument, so you need 
> > to handle that somehow, because otherwise this is going to underflow and 
> > set up the identifier with the last `InterestingIdentifierKind`.  There are 
> > two options:
> > - You could just make `initializeBuiltins` call some sort of 
> > `clearBuiltinID` method that resets `ObjCOrBuiltinID` to 0.  That has the 
> > advantage of making the "lifecycle" of this field much more 
> > straightforward.  For example, we probably ought to be asserting in these 
> > methods that we're not overwriting existing data; `clearBuiltinID` would 
> > know that it was okay to clear data, but only from a builtin.
> > - You could also just check for `NotBuiltin` and set `ObjCBuiltinID = 0` 
> > when you see it.
> > `initializeBuiltins` does call `setBuiltinID(NotBuiltin)` in order to reset 
> > any identifiers for which we have a  `-fno-builtin=X` argument, so you need 
> > to handle that somehow, because otherwise this is going to underflow and 
> > set up the identifier with the last `InterestingIdentifierKind`.  There are 
> > two options:
> > - You could just make `initializeBuiltins` call some sort of 
> > `clearBuiltinID` method that resets `ObjCOrBuiltinID` to 0.  That has the 
> > advantage of making the "lifecycle" of this field much more 
> > straightforward.  For example, we probably ought to be asserting in these 
> > methods that we're not overwriting existing data; `clearBuiltinID` would 
> > know that it was okay to clear data, but only from a builtin.
> 
> Would that be an additional step in `initializeBuiltins` or part of step #4? 
> I understants intuitively what the issue would be here if we didn't do that, 
> but I can't find a test case that goes through that step. Would be clearer to 
> me.
> 
> > - You could also just check for `NotBuiltin` and set `ObjCBuiltinID = 0` 
> > when you see it.
> 
> I don't think this can be done here. builtin::NotBuiltin is not known here in 
> IdentierTable.h (get a lots of build errors). So, this needs to be done in 
> `initializerBultins` method.
> Would that be an additional step in initializeBuiltins or part of step #4? I 
> understants intuitively what the issue would be here if we didn't do that, 
> but I can't find a test case that goes through that step. Would be clearer to 
> me.

Yeah, it'd be step 4: instead of calling 
`Table.get(Name).setBuiltinID(Builtin::NotBuiltin);`, it would call 
`Table.get(Name).clearBuiltinID()`.

...although while you're editing that code, please change `Table.get(Name)` to 
`NameIt->second`.  We've already looked up the `IdentifierInfo` and don't need 
to do it again.

> I don't think this can be done here. builtin::NotBuiltin is not known here in 
> IdentierTable.h (get a lots of build errors). So, this needs to be done in 
> initializerBultins method.

Okay, that works for me.  Please at least assert that the argument to 
`setBuiltinID` is not this value.  It's acceptable to assume that the value is 
0 — we're doing that already with the -1 / +1.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/IdentifierTable.h:85
+///  (including not_interesting).
+///   - The rest of the values represent builtin IDs (including not_builtin).
+static constexpr int FirstObjCKeywordID = 1;

The code below does not represent NotBuiltin (that's what adding and 
subtracting 1 does).



Comment at: clang/include/clang/Basic/IdentifierTable.h:89
+static constexpr int FirstInterestingIdentifierID = LastObjCKeywordID + 1;
+static constexpr int LastInterestingIdentifierID = LastObjCKeywordID + 
tok::NUM_INTERESTING_IDENTIFIERS;
+static constexpr int FirstBuiltinID = LastInterestingIdentifierID + 1;

I see that I had a bug in my suggestion: I had meant to write 
`LastInterestingIdentifierID = FirstInterestingIdentifierID + 
tok::NUM_INTERESTING_IDENTIFIERS - 2;` but I left it in terms of 
`LastObjCKeywordID` instead, making the ranges off by 1.  Your math fixes that; 
sorry about that.  I do think it would be clearer if each of these chained off 
the last one, the way I meant to have it, though.  So with your ranges (which 
leave space to explicitly represent `not_interesting`), that would look like 
`LastInterestingIdentifierID = FirstInterestingIdentifierID + 
tok::NUM_INTERESTING_IDENTIFIERS - 1;`.

I'm not going to push you to not represent `not_interesting`, since you seem to 
have deliberately changed things back that way, and I don't think it matters 
that much.  Although maybe you did that just because it didn't work in the code 
I gave you?  It would be more consistent with the other enums to not explicitly 
represent `not_interesting`.



Comment at: clang/include/clang/Basic/IdentifierTable.h:325
   void setBuiltinID(unsigned ID) {
-ObjCOrBuiltinID = ID + tok::NUM_OBJC_KEYWORDS;
-assert(ObjCOrBuiltinID - unsigned(tok::NUM_OBJC_KEYWORDS) == ID
-   && "ID too large for field!");
+ObjCOrBuiltinID = FirstBuiltinID + (ID - 1);
+assert(getBuiltinID() == ID && "ID too large for field!");

`initializeBuiltins` does call `setBuiltinID(NotBuiltin)` in order to reset any 
identifiers for which we have a  `-fno-builtin=X` argument, so you need to 
handle that somehow, because otherwise this is going to underflow and set up 
the identifier with the last `InterestingIdentifierKind`.  There are two 
options:
- You could just make `initializeBuiltins` call some sort of `clearBuiltinID` 
method that resets `ObjCOrBuiltinID` to 0.  That has the advantage of making 
the "lifecycle" of this field much more straightforward.  For example, we 
probably ought to be asserting in these methods that we're not overwriting 
existing data; `clearBuiltinID` would know that it was okay to clear data, but 
only from a builtin.
- You could also just check for `NotBuiltin` and set `ObjCBuiltinID = 0` when 
you see it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Basic/Builtins.cpp:33
 static constexpr Builtin::Info BuiltinInfo[] = {
-{"not a builtin function", nullptr, nullptr, nullptr, 
HeaderDesc::NO_HEADER,
+#define INTERESTING_IDENTIFIER(ID) 
\
+  {#ID, nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER, ALL_LANGUAGES},

zahiraam wrote:
> From your comment in Builtin.h: "You shouldn't muddle this into Builtin::ID."
> This means this shouldn't happen here. It should be done in 
> IdentiferTable::AddKeywords as I had done previously?
You shouldn't be adding entries for the interesting keywords to the builtins 
table, no.  This table is indexed by Builtin::ID.

I agree that `AddKeywords` seems like the right place to set up the interesting 
identifiers rather than `initializeBuiltins`.



Comment at: clang/lib/Basic/Builtins.cpp:138
+  // Step #2: mark all target-independent builtins with their ID's.
+  for (unsigned i = FirstBuiltinID; i != Builtin::FirstTSBuiltin; ++i)
 if (builtinIsSupported(BuiltinInfo[i], LangOpts)) {

You shouldn't use FirstBuiltinID and FirstInterestingIdentifierID as the 
starting points here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/Builtins.h:65
 enum ID {
+#define INTERESTING_IDENTIER(ID, TYPE, ATTRS) ID,
   NotBuiltin  = 0,  // This is not a builtin function.

You shouldn't muddle this into `Builtin::ID`.



Comment at: clang/include/clang/Basic/IdentifierTable.h:79
+static constexpr int FirstInterestingIdentifierID = 1;
+static constexpr int FirstBuiltinID = 3;
 

```
// The "layout" of ObjCOrBuiltinID is:
//   - The first value (0) represents "not a special identifier".
//   - The next (NUM_OBJC_KEYWORDS - 1) values represent ObjCKeywordKinds (not
//  including objc_not_keyword).
//   - The next (NUM_INTERESTING_IDENTIFIERS - 1) values represent 
InterestingIdentifierKinds
//  (not including not_interesting).
//   - The rest of the values represent builtin IDs (not including not_builtin).
static constexpr int FirstObjCKeywordID = 1;
static constexpr int LastObjCKeywordID = FirstObjCKeywordID + 
tok::NUM_OBJC_KEYWORDS - 2;
static constexpr int FirstInterestingIdentifierID = LastObjCKeywordID + 1;
static constexpr int LastInterestingIdentifierID = LastObjCKeywordID + 
tok::NUM_INTERESTING_IDENTIFIERS - 2;
static constexpr int FirstBuiltinID = LastInterestingIdentifierID + 1;
```



Comment at: clang/include/clang/Basic/IdentifierTable.h:295
   tok::ObjCKeywordKind getObjCKeywordID() const {
 if (ObjCOrBuiltinID < tok::NUM_OBJC_KEYWORDS)
   return tok::ObjCKeywordKind(ObjCOrBuiltinID);

```
  static_assert(FirstObjCKeywordID == 1, "hard-coding this assumption to 
simplify code");
  if (ObjCOrBuiltinID <= LastObjCKeywordID)
return tok::ObjCKeywordKind(ObjCOrBuiltinID);
  else
return tok::objc_not_keyword;
```



Comment at: clang/include/clang/Basic/IdentifierTable.h:306
   unsigned getBuiltinID() const {
-if (ObjCOrBuiltinID >= tok::NUM_OBJC_KEYWORDS)
-  return ObjCOrBuiltinID - tok::NUM_OBJC_KEYWORDS;
+if (ObjCOrBuiltinID >=
+(tok::NUM_OBJC_KEYWORDS + tok::NUM_INTERESTING_IDENTIFIERS))

```
if (ObjCOrBuiltinID >= FirstBuiltinID)
  return 1 + (ObjCOrBuiltinID - FirstBuiltinID);
else
  return 0;
```



Comment at: clang/include/clang/Basic/IdentifierTable.h:314
   void setBuiltinID(unsigned ID) {
-ObjCOrBuiltinID = ID + tok::NUM_OBJC_KEYWORDS;
-assert(ObjCOrBuiltinID - unsigned(tok::NUM_OBJC_KEYWORDS) == ID
-   && "ID too large for field!");
+ObjCOrBuiltinID =
+ID + tok::NUM_OBJC_KEYWORDS + tok::NUM_INTERESTING_IDENTIFIERS;

```
  assert(ID != NotBuiltin);
  ObjCOrBuiltinID = FirstBuiltinID + (ID - 1);
  assert(getBuiltinID() == ID && "ID too large for field!");
```



Comment at: clang/include/clang/Basic/IdentifierTable.h:323
+  unsigned getInterestingIdentifierID() {
+unsigned LowerID = tok::NUM_INTERESTING_IDENTIFIERS + 
tok::NUM_OBJC_KEYWORDS;
+unsigned UpperID = tok::NUM_INTERESTING_IDENTIFIERS + 
tok::NUM_OBJC_KEYWORDS +

```
tok::InterestingIdentifierKind getInterestingIdentifierID() const {
  if (ObjCOrBuiltinID >= FirstInterestingIdentifierID && ObjCOrBuiltinID <= 
LastInterestingIdentifierID)
return tok::InterestingIdentifierKind(1 + (ObjCOrBuiltinID - 
FirstInterestingIdentifierID));
  else
return tok::not_interesting;
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152818: [Clang] Fix assertion when pragma FENV_ACCESS is used with a throw function.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I'd guess we're not resetting the pragma state appropriately when instantiating 
templates.  Expressions should be governed by the pragmas in scope at the point 
of definition, not at the point of instantiation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152818/new/

https://reviews.llvm.org/D152818

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152689: [NFC] Remove dead conditionals

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/CodeGen/CGObjCMac.cpp:3812
   llvm::GlobalVariable *GV;
-  if (ForClass)
-GV =

aaron.ballman wrote:
> I agree this is dead code (see line 3777-3778 above), but @rjmccall is it a 
> bug that we bail out with a null value (e.g, is the correct fix to remove the 
> above lines)?
No, I don't think this is a bug, and that would not be the right fix anyway.  
`ForClass` means we're building the metaclass, and subclasses do not introduce 
new ivars in the metaclass.  The comment is saying that GCC declares ivars for 
the class structure when emitting metaclasses for root classes, but if we 
haven't been doing that for 15 years, I don't see a reason to start.  Certainly 
the runtime doesn't require it in order to work.  So the patch as written looks 
good.

For what it's worth, this code is all but dead for more basic reasons. 
`CGObjCMac` is the legacy ObjC ABI, which is no longer used on any platform 
that's even vaguely supported by Apple.  The last platform we shipped using 
this ABI was i386 macOS; we haven't sold Macs that only supported i386 since 
2006, and we dropped the i386 userspace entirely in macOS Catalina (2019).  So 
the only reason for this code to exist is to allow people to either compile 
programs for very old hardware or intentionally use a 32-bit ABI on newer 
hardware running an old OS.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152689/new/

https://reviews.llvm.org/D152689

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152096: [Clang] Check for abstract parameters only when functions are defined.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/test/SemaObjCXX/parameters.mm:14-17
+struct test2 { virtual void foo() = 0; };
 @interface Test2
-- (void) foo: (test2) foo; // expected-error {{parameter type 'test2' is an 
abstract class}}
+- (void) foo: (test2) foo ;
 @end

aaron.ballman wrote:
> I think this change makes sense for Objective-C as well (this is a 
> declaration and not a definition, at least as I understand things), but 
> pinging @rjmccall just in case I'm wrong.
No, that's right, methods in `@interface`s and `@protocol`s are just 
declarations.

Consistency with the base standard seems like the right way to go here, even 
those I personally find this rule rather silly.  (It's one thing  to allow this 
for deleted methods, but allowing it for declarations?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152096/new/

https://reviews.llvm.org/D152096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> I would like to clarify the second point to make sure that I'm incorporating 
> that suggestion - the checks on lines 25, 26 & 27 are for actual calls - 
> since now the signature matches the VTT type there's no arg setup prior to 
> the call, it's a direct passing of the global. Were you thinking about 
> something else here / am I missing somethign obvious?

Oh, sorry, somehow I glazed over those three lines.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150746/new/

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The process is pretty lightweight; I'd recommend just doing it if you expect to 
make more than one patch.  But we don't have a policy against committing 
patches for other people as long as there's clear attribution in the commit.

I do think you could successfully test the basic code patterns with passing VTT 
arguments.  Your test is solely testing that these constructors and destructors 
are being declared with the right signature, but it doesn't test any calls to 
them.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150746/new/

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151515: [CodeGen] add additional cast when checking call arguments

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yeah, I agree with Eli, there should be a cast here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151515/new/

https://reviews.llvm.org/D151515

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

One slight miscommunication.  Otherwise this LGTM, thank you.




Comment at: clang/docs/LanguageExtensions.rst:824
+provide native architectural support for arithmetic on these formats. These
+targets are noted in the lists of supported targets above.
+

You can drop this paragraph, it's no longer necessary.  I should've been 
clearer that I was suggesting this, sorry.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150913/new/

https://reviews.llvm.org/D150913

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:852
 ``double`` when passed to ``printf``, so the programmer must explicitly cast 
it to
 ``double`` before using it with an ``%f`` or similar specifier.
 

codemzs wrote:
> pengfei wrote:
> > rjmccall wrote:
> > > pengfei wrote:
> > > > rjmccall wrote:
> > > > > Suggested rework:
> > > > > 
> > > > > ```
> > > > > Clang supports three half-precision (16-bit) floating point types: 
> > > > > ``__fp16``,
> > > > > ``_Float16`` and ``__bf16``.  These types are supported in all 
> > > > > language
> > > > > modes, but not on all targets:
> > > > > 
> > > > > - ``__fp16`` is supported on every target.
> > > > > 
> > > > > - ``_Float16`` is currently supported on the following targets:
> > > > >   * 32-bit ARM (natively on some architecture versions)
> > > > >   * 64-bit ARM (AArch64) (natively on ARMv8.2a and above)
> > > > >   * AMDGPU (natively)
> > > > >   * SPIR (natively)
> > > > >   * X86 (if SSE2 is available; natively if AVX512-FP16 is also 
> > > > > available)
> > > > > 
> > > > > - ``__bf16`` is currently supported on the following targets:
> > > > >   * 32-bit ARM
> > > > >   * 64-bit ARM (AArch64)
> > > > >   * X86 (when SSE2 is available)
> > > > > 
> > > > > (For X86, SSE2 is available on 64-bit and all recent 32-bit 
> > > > > processors.)
> > > > > 
> > > > > ``__fp16`` and ``_Float16`` both use the binary16 format from IEEE
> > > > > 754-2008, which provides a 5-bit exponent and an 11-bit significand
> > > > > (counting the implicit leading 1).  ``__bf16`` uses the `bfloat16
> > > > > `_ 
> > > > > format,
> > > > > which provides an 8-bit exponent and an 8-bit significand; this is 
> > > > > the same
> > > > > exponent range as `float`, just with greatly reduced precision.
> > > > > 
> > > > > ``_Float16`` and ``__bf16`` follow the usual rules for arithmetic
> > > > > floating-point types.  Most importantly, this means that arithmetic 
> > > > > operations
> > > > > on operands of these types are formally performed in the type and 
> > > > > produce
> > > > > values of the type.  ``__fp16`` does not follow those rules: most 
> > > > > operations
> > > > > immediately promote operands of type ``__fp16`` to ``float``, and so
> > > > > arithmetic operations are defined to be performed in ``float`` and so 
> > > > > result in
> > > > > a value of type ``float`` (unless further promoted because of other 
> > > > > operands).
> > > > > See below for more information on the exact specifications of these 
> > > > > types.
> > > > > 
> > > > > Only some of the supported processors for ``__fp16`` and ``__bf16`` 
> > > > > offer
> > > > > native hardware support for arithmetic in their corresponding formats.
> > > > > The exact conditions are described in the lists above.  When 
> > > > > compiling for a
> > > > > processor without native support, Clang will perform the arithmetic in
> > > > > ``float``, inserting extensions and truncations as necessary.  This 
> > > > > can be
> > > > > done in a way that exactly emulates the behavior of hardware support 
> > > > > for
> > > > > arithmetic, but it can require many extra operations.  By default, 
> > > > > Clang takes
> > > > > advantage of the C standard's allowances for excess precision in 
> > > > > intermediate
> > > > > operands in order to eliminate intermediate truncations within 
> > > > > statements.
> > > > > This is generally much faster but can generate different results from 
> > > > > strict
> > > > > operation-by-operation emulation.
> > > > > 
> > > > > The use of excess precision can be independently controlled for these 
> > > > > two
> > > > > types with the ``-ffloat16-excess-precision=`` and
> > > > > ``-fbfloat16-excess-precision=`` options.  Valid values include:
> > > > > - ``none`` (meaning to perform strict operation-by-operation 
> > > > > emulation)
> > > > > - ``standard`` (meaning that excess precision is permitted under the 
> > > > > rules
> > > > >   described in the standard, i.e. never across explicit casts or 
> > > > > statements)
> > > > > - ``fast`` (meaning that excess precision is permitted whenever the
> > > > >   optimizer sees an opportunity to avoid truncations; currently this 
> > > > > has no
> > > > >   effect beyond ``standard``)
> > > > > 
> > > > > The ``_Float16`` type is an interchange floating type specified in
> > > > >  ISO/IEC TS 18661-3:2015 ("Floating-point extensions for C").  It will
> > > > > be supported on more targets as they define ABIs for it.
> > > > > 
> > > > > The ``__bf16`` type is a non-standard extension, but it generally 
> > > > > follows
> > > > > the rules for arithmetic interchange floating types from ISO/IEC TS
> > > > > 18661-3:2015.  In previous versions of Clang, it was a storage-only 
> > > > > type
> > > > > that forbade arithmetic operations.  It will be supported on more 
> > > > > targets
> > 

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Right, that's been my thinking, too.  The CVR qualifiers are all "view" 
qualifiers: both the underlying object and the reference to it are assumed to 
be the same independent of qualifiers, and the qualifiers just affect the rules 
around accesses.  C++'s rules around qualifiers in conversions/casts make sense 
for view qualifiers, but a lot of our extended qualifiers aren't view 
qualifiers, so it makes sense that the rules would be different.  Conversions 
between overlapping address spaces make sense to do with `static_cast` (or even 
an implicit conversion if the source AS is a subset).  Conversions between 
non-overlapping address spaces should probably just not be allowed, honestly, 
the same way we don't allow conversions between different ObjC ARC qualifiers.  
There are plenty of less dangerous ways to reinterpret bits if someone's sure 
they know what they're doing.

In fact, C already formalizes this distinction: `_Atomic` behaves like a 
qualifier in many ways, but the language doesn't formally consider it one, and 
part of the reason for that is that it *doesn't* behave like a qualifier in 
some *other* ways.  For example, you cannot convert a `T *` to an `_Atomic(T) 
*`, and the object representations don't necessarily match.

In fact, it's not really true of any of the other qualifiers we support: 
address spaces, the Objective-C ARC qualifiers, the ptrauth qualifiers that 
will eventually be upstreamed, etc.

, that conversions between overlapping address spaces are more like hierarchy 
conversions than qualification conversions.  The C++ rules around casting 
qualifiers make sense for the "variant view" qualifiers

should be limited to qualifiers

should probably be doable with `static_cast`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151087/new/

https://reviews.llvm.org/D151087

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

As far as I'm concerned as editor of the Itanium ABI, the ABI treatment of 
trivial-for-the-purposes-of-calls classes is purely a psABI matter, and the 
Itanium ABI's wording around empty classes is merely a suggestion if the psABI 
doesn't have more specific rules (because empty structs are normally invalid in 
C).  Do what you think is best for your ABI.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151298/new/

https://reviews.llvm.org/D151298

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151076: [IRGen] Handle infinite cycles in findDominatingStoreToReturnValue.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Nice catch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151076/new/

https://reviews.llvm.org/D151076

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:852
 ``double`` when passed to ``printf``, so the programmer must explicitly cast 
it to
 ``double`` before using it with an ``%f`` or similar specifier.
 

pengfei wrote:
> rjmccall wrote:
> > Suggested rework:
> > 
> > ```
> > Clang supports three half-precision (16-bit) floating point types: 
> > ``__fp16``,
> > ``_Float16`` and ``__bf16``.  These types are supported in all language
> > modes, but not on all targets:
> > 
> > - ``__fp16`` is supported on every target.
> > 
> > - ``_Float16`` is currently supported on the following targets:
> >   * 32-bit ARM (natively on some architecture versions)
> >   * 64-bit ARM (AArch64) (natively on ARMv8.2a and above)
> >   * AMDGPU (natively)
> >   * SPIR (natively)
> >   * X86 (if SSE2 is available; natively if AVX512-FP16 is also available)
> > 
> > - ``__bf16`` is currently supported on the following targets:
> >   * 32-bit ARM
> >   * 64-bit ARM (AArch64)
> >   * X86 (when SSE2 is available)
> > 
> > (For X86, SSE2 is available on 64-bit and all recent 32-bit processors.)
> > 
> > ``__fp16`` and ``_Float16`` both use the binary16 format from IEEE
> > 754-2008, which provides a 5-bit exponent and an 11-bit significand
> > (counting the implicit leading 1).  ``__bf16`` uses the `bfloat16
> > `_ format,
> > which provides an 8-bit exponent and an 8-bit significand; this is the same
> > exponent range as `float`, just with greatly reduced precision.
> > 
> > ``_Float16`` and ``__bf16`` follow the usual rules for arithmetic
> > floating-point types.  Most importantly, this means that arithmetic 
> > operations
> > on operands of these types are formally performed in the type and produce
> > values of the type.  ``__fp16`` does not follow those rules: most operations
> > immediately promote operands of type ``__fp16`` to ``float``, and so
> > arithmetic operations are defined to be performed in ``float`` and so 
> > result in
> > a value of type ``float`` (unless further promoted because of other 
> > operands).
> > See below for more information on the exact specifications of these types.
> > 
> > Only some of the supported processors for ``__fp16`` and ``__bf16`` offer
> > native hardware support for arithmetic in their corresponding formats.
> > The exact conditions are described in the lists above.  When compiling for a
> > processor without native support, Clang will perform the arithmetic in
> > ``float``, inserting extensions and truncations as necessary.  This can be
> > done in a way that exactly emulates the behavior of hardware support for
> > arithmetic, but it can require many extra operations.  By default, Clang 
> > takes
> > advantage of the C standard's allowances for excess precision in 
> > intermediate
> > operands in order to eliminate intermediate truncations within statements.
> > This is generally much faster but can generate different results from strict
> > operation-by-operation emulation.
> > 
> > The use of excess precision can be independently controlled for these two
> > types with the ``-ffloat16-excess-precision=`` and
> > ``-fbfloat16-excess-precision=`` options.  Valid values include:
> > - ``none`` (meaning to perform strict operation-by-operation emulation)
> > - ``standard`` (meaning that excess precision is permitted under the rules
> >   described in the standard, i.e. never across explicit casts or statements)
> > - ``fast`` (meaning that excess precision is permitted whenever the
> >   optimizer sees an opportunity to avoid truncations; currently this has no
> >   effect beyond ``standard``)
> > 
> > The ``_Float16`` type is an interchange floating type specified in
> >  ISO/IEC TS 18661-3:2015 ("Floating-point extensions for C").  It will
> > be supported on more targets as they define ABIs for it.
> > 
> > The ``__bf16`` type is a non-standard extension, but it generally follows
> > the rules for arithmetic interchange floating types from ISO/IEC TS
> > 18661-3:2015.  In previous versions of Clang, it was a storage-only type
> > that forbade arithmetic operations.  It will be supported on more targets
> > as they define ABIs for it.
> > 
> > The ``__fp16`` type was originally an ARM extension and is specified
> > by the `ARM C Language Extensions 
> > `_.
> > Clang uses the ``binary16`` format from IEEE 754-2008 for ``__fp16``,
> > not the ARM alternative format.  Operators that expect arithmetic operands
> > immediately promote ``__fp16`` operands to ``float``.
> > 
> > It is recommended that portable code use ``_Float16`` instead of ``__fp16``,
> > as it has been defined by the C standards committee and has behavior that is
> > more familiar to most programmers.
> > 
> > Because ``__fp16`` operands are always immediately promoted to ``float``, 
> > the
> > common real type of ``__fp16`` and 

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Apologies for misunderstanding what this patch was doing.  This all seems 
reasonable, and the code changes look good.  I think the documentation needs 
significant reorganization; I've attached a draft.  Please review for 
correctness.




Comment at: clang/docs/LanguageExtensions.rst:852
 ``double`` when passed to ``printf``, so the programmer must explicitly cast 
it to
 ``double`` before using it with an ``%f`` or similar specifier.
 

Suggested rework:

```
Clang supports three half-precision (16-bit) floating point types: ``__fp16``,
``_Float16`` and ``__bf16``.  These types are supported in all language
modes, but not on all targets:

- ``__fp16`` is supported on every target.

- ``_Float16`` is currently supported on the following targets:
  * 32-bit ARM (natively on some architecture versions)
  * 64-bit ARM (AArch64) (natively on ARMv8.2a and above)
  * AMDGPU (natively)
  * SPIR (natively)
  * X86 (if SSE2 is available; natively if AVX512-FP16 is also available)

- ``__bf16`` is currently supported on the following targets:
  * 32-bit ARM
  * 64-bit ARM (AArch64)
  * X86 (when SSE2 is available)

(For X86, SSE2 is available on 64-bit and all recent 32-bit processors.)

``__fp16`` and ``_Float16`` both use the binary16 format from IEEE
754-2008, which provides a 5-bit exponent and an 11-bit significand
(counting the implicit leading 1).  ``__bf16`` uses the `bfloat16
`_ format,
which provides an 8-bit exponent and an 8-bit significand; this is the same
exponent range as `float`, just with greatly reduced precision.

``_Float16`` and ``__bf16`` follow the usual rules for arithmetic
floating-point types.  Most importantly, this means that arithmetic operations
on operands of these types are formally performed in the type and produce
values of the type.  ``__fp16`` does not follow those rules: most operations
immediately promote operands of type ``__fp16`` to ``float``, and so
arithmetic operations are defined to be performed in ``float`` and so result in
a value of type ``float`` (unless further promoted because of other operands).
See below for more information on the exact specifications of these types.

Only some of the supported processors for ``__fp16`` and ``__bf16`` offer
native hardware support for arithmetic in their corresponding formats.
The exact conditions are described in the lists above.  When compiling for a
processor without native support, Clang will perform the arithmetic in
``float``, inserting extensions and truncations as necessary.  This can be
done in a way that exactly emulates the behavior of hardware support for
arithmetic, but it can require many extra operations.  By default, Clang takes
advantage of the C standard's allowances for excess precision in intermediate
operands in order to eliminate intermediate truncations within statements.
This is generally much faster but can generate different results from strict
operation-by-operation emulation.

The use of excess precision can be independently controlled for these two
types with the ``-ffloat16-excess-precision=`` and
``-fbfloat16-excess-precision=`` options.  Valid values include:
- ``none`` (meaning to perform strict operation-by-operation emulation)
- ``standard`` (meaning that excess precision is permitted under the rules
  described in the standard, i.e. never across explicit casts or statements)
- ``fast`` (meaning that excess precision is permitted whenever the
  optimizer sees an opportunity to avoid truncations; currently this has no
  effect beyond ``standard``)

The ``_Float16`` type is an interchange floating type specified in
 ISO/IEC TS 18661-3:2015 ("Floating-point extensions for C").  It will
be supported on more targets as they define ABIs for it.

The ``__bf16`` type is a non-standard extension, but it generally follows
the rules for arithmetic interchange floating types from ISO/IEC TS
18661-3:2015.  In previous versions of Clang, it was a storage-only type
that forbade arithmetic operations.  It will be supported on more targets
as they define ABIs for it.

The ``__fp16`` type was originally an ARM extension and is specified
by the `ARM C Language Extensions 
`_.
Clang uses the ``binary16`` format from IEEE 754-2008 for ``__fp16``,
not the ARM alternative format.  Operators that expect arithmetic operands
immediately promote ``__fp16`` operands to ``float``.

It is recommended that portable code use ``_Float16`` instead of ``__fp16``,
as it has been defined by the C standards committee and has behavior that is
more familiar to most programmers.

Because ``__fp16`` operands are always immediately promoted to ``float``, the
common real type of ``__fp16`` and ``_Float16`` for the purposes of the usual
arithmetic conversions is ``float``.

A literal can be given ``_Float16`` type using the suffix ``f16``. For example,
``3.14f16``.

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

It's weird to have C-style casts that can't be done with any combination of 
named casts, so I think this makes some sense.  I do think it should be limited 
to numbered address spaces of the same size, though, rather than basing it on 
whether we're in OpenCL mode.  Target address spaces should not generally be 
forced into this rule.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151087/new/

https://reviews.llvm.org/D151087

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I see.  Yes, in that case I think you're right that we can't test this yet — 
only base-subobject ctors and dtors get VTT parameters, we only emit calls to 
those from other ctors and dtors, and those ctors and dtors will always have 
their own stores to the v-table slot that will be invalid IR.  So I agree with 
the plan of building up an XFAILed test.  You should be able to locally build 
up that test by just adding an early return to 
`CodeGenFunction::InitializeVTablePointer` — obviously that will break a bunch 
of other tests, but as long as you don't commit it, you can create a fairly 
complete test case that at least should work in the future.  I find it helpful 
to do that as you're going instead of going back at the end of a patch series 
and trying to add tests for all the cases you implemented.  Please test (1) the 
declarations of base-subobject ctors/dtors, (2) the initial passing of the VTT 
argument from complete-object to base-subobject ctors/dtors, and (3) the 
forwarding of VTT arguments in base-subobject ctors/dtors to the ctors/dtors of 
their own base subobjects.

The actual code LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150746/new/

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

It's well-formed as IR, just not semantically valid, right?  As long as it's 
not actually crashing in the verifier, please test as much as you reasonably 
can that's correct, like that the constructor and destructor functions take 
something in the right address space, and that calls to them pass a valid VTT.  
And then yeah, when you pull a v-table out and try to assign it, that much will 
be incorrect until your next fix, but that's okay.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150746/new/

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148089: [clang][CodeGen] Break up TargetInfo.cpp [1/8]

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Sergei, feel free to start landing patches like this one that were already 
approved.  You don't need the entire sequence to be approved first.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148089/new/

https://reviews.llvm.org/D148089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/IdentifierTable.h:316
+  unsigned getInterestingIdentifierID() {
+if (ObjCOrBuiltinID >= 1341 &&  ObjCOrBuiltinID < 1343)
+  return ObjCOrBuiltinID;

This is closer to the right approach, thanks.  I think the best way to do this 
is to put the ObjC keywords first, then the interesting identifiers, then the 
builtins.  That means the builtin ID code above should check for / add / 
subtract `tok::NUM_OBJC_KEYWORDS + tok::NUM_INTERESTING_IDENTIFIERS`; please 
add a `FirstBuiltinID` constant for that in this class so that we can more 
easily rework things in the future (e.g. if we decide to stuff more things into 
this field).  And then the code for InterestingIdentifierID should add/subtract 
`tok::NUM_OBJC_KEYWORDS`; again, please add a `FirstInterestingIdentifier` 
constant for that.

Please make these two functions use `tok::InterestingIdentifierKind`.  For 
`getInterestingIdentifierID()`, you'll have to decide how you want to represent 
that something isn't an interesting identifier. `ObjCKeywordKind` has a special 
enumerator (`not_keyword`) at value 0, so you could do the same thing here, but 
I think it might be better to use `llvm::Optional`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Basic/Targets/X86.cpp:381
 
 HasBFloat16 = SSELevel >= SSE2;
 

pengfei wrote:
> I'm not sure if I understand the meaning of `HasFullBFloat16`. If it is used 
> for target that supports arithmetic `__bf16`, we should not use `+fullbf16` 
> but always enable it for SSE2, i.e., `HasFullBFloat16 = SSELevel >= SSE2`. 
> Because X86 GCC already supports arithmetic for `__bf16`.
> 
> If this is used in the way like `HasLegalHalfType`, we should enable it once 
> we have a full BF16 ISA on X86. `fullbf16` doesn't make much sense to me.
At the moment, we haven't done the work to emulate BFloat16 arithmetic in any 
of the three ways we can do that: Clang doesn't promote it in IRGen, LLVM 
doesn't promote it in legalization, and we don't have compiler-rt functions for 
it.  If we emit these instructions, they'll just sail through LLVM and fail in 
the backend.  So in the short term, we have to restrict this to targets that 
directly support BFloat16 arithmetic in hardware, which doesn't include x86.

Once we have that emulation support, I agree that the x86 targets should enable 
this whenever they would enable `__bf16`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150913/new/

https://reviews.llvm.org/D150913

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits