Page MenuHomePhabricator

palves (Pedro Alves)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 30 2014, 7:28 AM (385 w, 5 d)

Recent Activity

May 12 2021

palves added a comment to D100404: Add Global support for #pragma clang attributes.
May 12 2021, 7:19 AM

May 5 2021

palves added a comment to D100404: Add Global support for #pragma clang attributes.

On 5/2/21 2:18 PM, Aaron Ballman via Phabricator wrote:

aaron.ballman added a comment.

In D100404#2731431 https://reviews.llvm.org/D100404#2731431, @palves wrote:

In D100404#2729968 https://reviews.llvm.org/D100404#2729968, @aaron.ballman wrote:

FWIW, I share the concern that having a global scope for this feature instead of explicit push/pop scopes is potentially dangerous. The ergonomics of the feature make it likely the user will be applying attributes to things they had no reason to believe they were adding attributes to because this effectively means "add attributes to everything from here on out, without restriction". The fact that the user has to type *less* code to get this more dangerous behavior is a worry.

C and C++ have plenty of ways to shoot oneself in the foot.

That doesn't give us licence to add more with our extensions. I was concerned about this feature when it was first introduced, because of how incredibly easy it is to accidentally mark things you don't intend to mark. The use of very explicit scopes alleviated that concern.

Heck, pointers are expressed with a single character ('*').
Putting "using namespace std;" in some central header is a bad idea. Etc. Yet, the languages don't prevent these things,
or try to make them harder to type.

Also, I don't see how dropping 4 characters ("push") makes it any more "dangerous" (whatever dangerous means). I mean, users can already put:

Because today, push requires a pop or you get an error. So accidentally getting the scope wrong is a loud issue for the programmer to respond to. What is being proposed here can accidentally apply attributes to *anything in the TU* with no notice that you've done something wrong. What's more, it's now easy to get into that situation by omission -- if you forget to write "push", your code compiles and works fine. Then you go and add more declarations *and who knows what happens*, but sometime later the programmer will likely discover that they missed the "push". That's what I mean by "more dangerous".

Sorry for the delay responding.

May 5 2021, 5:46 AM

May 1 2021

palves added inline comments to D100404: Add Global support for #pragma clang attributes.
May 1 2021, 12:36 PM
palves added a comment to D100404: Add Global support for #pragma clang attributes.

FWIW, I share the concern that having a global scope for this feature instead of explicit push/pop scopes is potentially dangerous. The ergonomics of the feature make it likely the user will be applying attributes to things they had no reason to believe they were adding attributes to because this effectively means "add attributes to everything from here on out, without restriction". The fact that the user has to type *less* code to get this more dangerous behavior is a worry.

May 1 2021, 12:28 PM

Apr 28 2021

palves added inline comments to D100404: Add Global support for #pragma clang attributes.
Apr 28 2021, 6:36 AM

Apr 22 2021

palves added a comment to D100404: Add Global support for #pragma clang attributes.

Please review again, I have changed the patch description, and revised to the comments.

This new patch will accept #pragma clang attribute (...) without push/pop, and store the filter details into a global attribute group.

Apr 22 2021, 12:43 PM

Apr 20 2021

palves added a comment to D100404: Add Global support for #pragma clang attributes.

The reason I added the #pca () syntax in D53621 https://reviews.llvm.org/D53621 was so that you could add multiple attributes to a single pushed scope in a context where you knew
exactly which scope you were adding them to (i.e., where it immediately follows a #pca push). I don't think it makes sense to use it when you aren't aware of the #pca context. There really
should never be "lots of code" between a #pca push and a #pca ().

Apr 20 2021, 10:50 AM
palves added a comment to D100404: Add Global support for #pragma clang attributes.

Using a global scope is more complicated. We need to properly handle Erik's concerns about nested namespaces.

Apr 20 2021, 10:36 AM
palves added a comment to D100404: Add Global support for #pragma clang attributes.

erik.pilkington added a comment.
In D100404#2686753 https://reviews.llvm.org/D100404#2686753, @palves wrote:

If Clang supported a "global" scope, then you would be able to just use the "#pragma clang attribute (...)" syntax when you don't want to pop. Seems like the natural solution.

Hmm, the problem I see with that is that you have to know that nobody else push-ed a #pca scope that you're inadvertently adding to.

Apr 20 2021, 10:20 AM
palves added a comment to D100404: Add Global support for #pragma clang attributes.

Sure, it will work for me, though FWIW I think it's the worse option of the 3 potential solutions, and I'll be puzzled if we end with that UI.
Can anyone else speak up and state your opinions, please?

Apr 20 2021, 7:21 AM

Apr 13 2021

palves added a comment to D100404: Add Global support for #pragma clang attributes.

Back in https://reviews.llvm.org/D98201 we had a larger discussion where I gave my rationale for why I thought that Clang should instead not error out when the pop is missing, so I'm not going to repeat it here.

Apr 13 2021, 12:49 PM

Apr 12 2021

palves added a comment to D98201: [CUDA][HIP] Add #pragma clang force_cuda_device_globals {begin,end}.

On 12/04/21 18:37, Artem Belevich via Phabricator wrote:

tra added a comment.

@palves -- For some reason your reply didn't make it to the tracker. I guess phabricator does not handle email replies well.

I don't understand the desire to for extra syntax -- no other push/pop pragma requires separate syntax, they simply don't error out
if the pop is missing at the end of the translation unit.

I'd argue that a missing pop is an error ( extra one should be, too), and it's those other pragmas that are not doing the right thing.
That said, I'm not inherently opposed to allowing mismatched push/pop, but I'd prefer not to, if we can.

Apr 12 2021, 11:32 AM

Apr 7 2021

palves added a comment to D98201: [CUDA][HIP] Add #pragma clang force_cuda_device_globals {begin,end}.
In D98201#2674765, @tra wrote:

#2 - The unterminated #pragma clang attribute push error is also a blocker for our use case, because as I mentioned, we're putting the #pragma push in a header that is force-included, there's nowhere to put the corresponding pop.
This seems like a needless Clang restriction -- note that #pragma clang force_cuda_host_device begin does not error out with a missing corresponding pop, the pragma just ends up being in effect until the end of the translation unit. That's what we need here too.

Allowing pragma push to be unmatched, maybe with an explicit option to enable it, would probably be less controversial than adding a new pragma that duplicates existing functionality.

Apr 7 2021, 12:41 PM
palves added a comment to D98201: [CUDA][HIP] Add #pragma clang force_cuda_device_globals {begin,end}.
In D98201#2664431, @tra wrote:

@rsmith suggested that #pragma clang attribute push (__device__, apply_to = variable(is_global)) may already be able to do what this patch is attempting to do. Can you check if you can make it work for your tests?

Apr 7 2021, 6:29 AM

Mar 16 2021

palves added inline comments to D98201: [CUDA][HIP] Add #pragma clang force_cuda_device_globals {begin,end}.
Mar 16 2021, 4:35 PM
palves added a comment to D98201: [CUDA][HIP] Add #pragma clang force_cuda_device_globals {begin,end}.
In D98201#2629709, @tra wrote:

I believe the GDB testsuite is being compiled targeting the device/AMDGPU, and the device code accesses these global variables which need to be marked with __device__. This allows many of GDB testcases to be compiled for device unmodified.

Sorry, I still don't understand.

Mar 16 2021, 1:12 PM