- User Since
- Jan 18 2013, 11:30 AM (321 w, 6 d)
Tue, Mar 19
Mon, Mar 18
This patch LGTM.
Sorry, I've gotten trapped under a backlog and haven't had time to think much about this. I don't want to block progress here.
Thu, Mar 14
Wed, Mar 13
I remember this coming up 7-8 years ago. I intentionally decided against doing a copy/release when assigning to __weak because if the block wasn't already guaranteed to be copied then it was probably better to crash than to silently assign a value that's about to be deallocated. Note that copying the block we assign into wb doesn't change anything about the block stored in b.
Tue, Mar 12
Using internal linkage instead of private makes sense to me. Even if we wanted to use private linkage, it never made sense to be doing this with \01l instead of just setting the linkage properly. Maybe this was implemented before LLVM directly supported private linkage, but that hasn't been a constraint in 10+ years.
Sat, Mar 9
Thu, Mar 7
Wed, Mar 6
Looks great, thanks!
Tue, Mar 5
This test file will probably grow over time, so please add a CHECK-LABEL line to the test case to make sure you're checking the body of foo(). Also, you'll probably need to allow the name %ref.tmp to be stripped for this test to pass in release builds; please go ahead and test a more complete pattern, including the alloca and the store of 1 to it. Otherwise LGTM.
Mon, Mar 4
In the abstract, it would be nice to warn about casts that always have UB because the address spaces don't overlap; however, I'm worried about how people might be using address spaces in strange ways today, knowing that they *do* overlap, only in ways that the compiler isn't smart enough for. I think we should just be permissive in non-OpenCL mode.
This seems eminently reasonable to warn about. I think people might legitimately complain about extending an existing warning to a new set of contexts without putting it under a new warning flag, though. How annoying would it be to clone the warnings and put them in a subgroup? You could also change the diagnostics to say "converting the result of an assignment to bool" instead of "using the result of an assignment as a condition".
Thu, Feb 28
Hmm. Yeah, Embedded C allows these casts, so contra my previous comment, I think we can't make them ill-formed outside of OpenCL mode.
Wed, Feb 27
I think the right model is that a builtin operator candidate exists for all possible address-space qualifiers on the pointer/reference pointee, and it should be straightforward to only actually add candidates for the qualifier used by the corresponding operand.
Tue, Feb 26
Well, it won't have a guarantee that it won't see unused annotations, but alright, I'm fine with this.
A couple tweaks, but otherwise LGTM.
Mon, Feb 25
Okay, one last minor request, then LGTM.
Fri, Feb 22
One comment, but otherwise LGTM.
Thu, Feb 21
I see, alright.
The correctness condition here is solely that we cannot allow the block literal to go out of scope before the variable that it is assigned to. (Local block literals with captures have lifetimes like C compound literals: until the end of the enclosing block, rather than the end of the full-expression.) So doing this blindly for assignments is problematic because you'd need to reason about relative lifetimes, but doing it for initialization should always be fine. I don't see any reason why address-taken-ness would matter as long as that condition holds.
I agree. There should just never be a copy expression if the capture is of reference type, and there should therefore be no need to special-case that in IRGen.
No, this LGTM.
Feb 15 2019
Okay, but it's not great design to have a kind of overloading that can't be resolved to an exact intended declaration even by an explicit cast. That's why I think making *optional* host/device typing is a good idea. And I strongly want to caution you against doing language design by just incrementally hacking at the compiler to progressively make more test-cases work, which is what it feels like you're doing.
It is totally unreasonable, at the time you are resolving a template argument, to investigate how the corresponding template parameter is used within the template and use that to shape how the template argument is resolved. That is simply not how the C++ template model works. Given that CODA doesn't distinguish between host and device functions in the type system, if you are going to have a rule here, it has to be based on, at most, (1) the current semantic context (which may not even be a function), (2) the template being specialized, and (3) the declarations in the template-argument set.
Feb 14 2019
But what we've just been talking about is not a validity rule, it's an overload-resolution rule. It's not *invalid* to use a device function as a template argument to a host function template (or to a class template, which of course is neither host nor device). All you need to do is to resolve otherwise-intractable overload ambiguities by matching with the host-ness of the current context, which there's probably already code to do for when an overload set is used as e.g. a function argument.
Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.
Okay. Is it acceptable for the annotation to simply disappear in this case? I don't really understand the purposes of annotations well enough to judge.
Feb 13 2019
Basically LGTM, especially if we need an emergency fix, but please consider addressing my comment before committing since I'd expect it to be straightforward to solve.
Feb 12 2019
Sorry, this is deep in the loop analysis code, and I don't remember anything about this stuff, so I'm not sure I'm going to be a useful reviewer.
I really don't understand this well enough to review.
Feb 11 2019
Good catch! Is there a reasonable way to just make FileCheck itself enforce this, or does that cause too many false positives with tests that are using e.g. x86_64 as the entire prefix?
Ah, thanks, seems like a reasonable fix.
Feb 8 2019
Feb 7 2019
Hmm. Richard, I've mostly let you stay out of this, but do you have any thoughts about the right manage to parse attributes here? We want to allow address_space attributes to be written in the method-qualifiers list, but when we do that, it's hard to avoid parsing arbitrary attributes and then potentially needing to move them to the declarator; also, doing that might mean propagating awkward information in to deal with of delayed attribute parsing.
LGTM, but why did I just get like a million different notifications of this from different services.
LGTM as well.
Also, why is this HIP-specific? I thought the toolchain was largely shared with CUDA and there were just a few runtime differences.
Two minor tweaks, but otherwise LGTM.
Feb 6 2019
Feb 5 2019
Moving parsed attributes between lists isn't unreasonable if that's what you have to do; we already do that when processing the ObjC ARC qualifiers. The ambiguity with function attributes is pretty much inherent.
Feb 4 2019
Richard felt that this was an odd special case, and I was happy to use the old language-designer's dodge of banning something today so that we can decide to allow it tomorrow. This isn't SFINAE-able.
Feb 1 2019
Asking for a minor tweak, but feel free to commit with that.
I agree: if a test doesn't seem to require the GNU extensions, let's not use -std=gnu++XX. Otherwise this LGTM.