Sure, adding an intrinsic doesn’t stop that by itself. But, first step can be to add the intrinsic, start emitting it for all address uses in frontends, and note that the direct use of @x as an i32* is deprecated (but, don’t yet prevent it).
A later step can be to make it actually invalid – e.g. by changing the IR type of @x to be some opaque token instead of i32* or ptr.
I think the problem is that the previous patches were not motivated by a clear understanding of the desired IR semantics. Without a clear definition of the semantics of various attributes, with regards to coroutines, patches can appear to be unprincipled workarounds.
But, I think we have basically come to a rough agreement that ‘readnone’ means “doesn’t read memory, but can access thread identity” – where the thread identity is almost always a constant, except within a pre-split coroutine function body.
My belief is that with that definition written properly in the LangRef, optimization pass modifications become justifiable, in a way they are not today.
Oh, and of course, this doesn’t only apply to readnone but also other memory-access-constraint attributes. In particular, writeonly and argmemonly.
Your suggestion makes sense. But I think my solution is not bad too. I think we could summarize these 2 methods as:
Ideally, both of the 2 methods would solve the problem from the perspective of users. So we need to talk about the implementation here. I prefer the second solution since we would be more confident to say that we’ve fixed the problem. But if we chose the first solution, I wouldn’t be so confident since I would be worrying if I missed some passes. In other words, I think the second solution is more self-contained and the first solution looks more open. (open might be the proper word here. Any suggestion?) Also I think the second solution is more consistent what we do with coroutine intrinsic. We would emit coroutine intrinsic in the frontend, do some transformation in CoroEarly pass and clean up all the remaining coroutine intrinsics in CoroCleanup pass. So I think the second solution is the proper solution.
@efriedma-quic @jyknight @rjmccall @nhaehnle @danilaml gentle ping~
I still think that a solution based on a new attribute (say, noread_thread_id) would be best from the perspective of LLVM IR.
The other half of the solution is a way for transforms to detect when noread_thread_id is redundant, which boils down to some isCoroutine check. I don’t feel strongly about how that should be done, exactly.
To my understanding, your suggestion means:
(1) Introduce a new attribute noread_thread_id (or anything similar)
(2) Implement the semantic for noread_thread_id.
(3) Emit noread_thread_id in the frontend instead of readnone if we are in C++ and its version is greater than 20. (Assume other languages don’t have similar problems now)
(4) Mark the intrinsic for TLS as noread_thread_id instead of readnone.
Do I understand right?
The pros of the approach is that its semantics look better and readnone could remain the original semantics.
The cons for the approach is about the implementation. We might need to convert many:
hasAttribute(ReadNone);
to
hasAttribute(ReadNone) || (hasAttribute(NoReadThreadID) && ! isCoroutine(CurrentFn));
So the problem comes to whether it is matter to keep the semantics of readnone?
(In my proposal, the semantics of readnone would become “read nothing except thread ID”.)
I prefer my solution slightly for the sake of engineering. And I am willing to hear opinions from experienced guys.
For concreteness, I was actually thinking that readnone means “does not access memory”, where “thread_id” is considered to not be part of memory. That is:
noread_thread_id
noread_thread_id in addition to readnone in the frontend for functions that are known to not read the thread ID implicitly or explicitly.readnone.hasAttribute(ReadNone) (e.g. to guard CSE) have to become hasAttribute(ReadNone) && (!isCoroutine(CurrentFn) || !hasAttribute(NoReadThreadID). Though, note that this isn’t the first time that happens. For example, we have a bunch of places that need to check hasAttribute(ReadNone) && !hasAttribute(Convergent). So there’s precedent.It ends up being functionally equivalent to how you interpreted what you wrote, but I’d argue that what I’m describing here is more composable. The way I like to think about it is that there are certain “capabilities” that code may be using: reading memory, writing memory, freeing memory, synchronization, accessing globals, indirect memory accesses; and attributes are used to express which of these capabilities code may be using. Capabilities can be thought of as (almost) a lattice, so defining the attributes in terms of “atomic” capabilities is better for composability.
I understand that there is a bit of tension here between what looks like raw software engineering pragmatism and formal semantics arguments. So I think I can mostly understand where you’re coming from, it’s just that personally, I’ve been burned too often by holes in semantics of IR which is why I fall more in the camp of “let’s please get the semantics right” 
Thank you for more-clearly restating the proposal from above.
My only nit is that I think we don’t actually need to implement noread_thread_id – yet. We DO need to agree on the model – that “memory” doesn’t include “thread_id”, and that a conceptual attribute noread_thread_id makes sense to add in the future.
But, to start out, we simply assume that all functions can read thread-id (aka: nothing has a noread_thread_id attribute). Thus, the checks start out as hasAttribute(ReadNone) && !isCoroutine(CurrentFn). This should be good enough, because we’ll apply the optimizations after coroutine lowering.
Later, if we decide the additional pre-lowering optimizations are useful, we add the attribute, and relax the checks to hasAttribute(ReadNone) && (hasAttribute(NoReadThreadId) || !isCoroutine(CurrentFn))
Thank you both for the clear proposal. I agree @jyknight here since if we decide to introduce noread_thread_id, we need to emit this one for many languages with many options. I am not complaining there are more works. I just feel they might not be necessary now and we could add them one day in case we find it is necessary. (As you said, I come from software engineering indeed : ))
@efriedma-quic @rjmccall Would you like to agree the proposal @jyknight raised? If yes, I think we get in consensus finally.
@efriedma-quic @rjmccall gentle ping~ would you like to agree the proposal @jyknight raised? (Solve the problem by converting checks “hasAttribute(ReadNone)” to hasAttribute(ReadNone) && !isCoroutine(CurrentFn))
I feel like it is better to move on after we get consensus.
I’m okay with that approach.
Sent: ⚙ D127383 Don't treat readnone call in presplit coroutine as not access memory and https://reviews.llvm.org/D125291. We could review/commit them in any order.
FWIW, I think this shows yet again that we need to model the different “side-effect kinds” explicitly. Blanked read/write only/none doesn’t work. The three extra categories (argmem, inaccesible, argmem+inaccesible) are also insufficient. What we need is a side-effect attribute:
side_effect(read:TLS,argmem; write: globals, cross-thread-registers; synchronizing("warp"))
And then we map what we have back to this rather than trying to shoehorn something into readnone/only that is not readnone/only. (Not to mention the other hacks and bugs we have because we don’t properly model cross-thread-register accesses, synchronization, …).
Update: I sent ⚙ D132352 Introduce noread_thread_id to address the thread identification problem in coroutines to implement the suggestion from @nhaehnle in Address thread identification problems with coroutine - #48 by nhaehnle due to @fhahn prefer a better semantic model.
I am sorry If this was already discussed (or there is a better place to ask), but I couldn’t find the answer.
Like in the following example (Compiler Explorer), compiler generated function tls_init accesses tls_guard (TLS var) directly, without llvm.threadlocal.address intrinsic.
Is that on purpose or we should/can use the intrinsic here as well?
That looks like a bug to me. Clang should go through llvm.threadlocal.address for all TLS accesses.
Yes, this sounds like a bug.
@nikic @ChuanqiXu Thanks for the quick answer! I will post a fix and add you as reviewers.