<div dir="ltr"><div dir="ltr">I view what happened is that we've introduced effectively a new form of IR (pre-coroutine-split IR), yet when doing so, we failed to realize the impact that a "suspend" allowing "thread identity" to change would have on the rest of LLVM. So, oops! And now we need to design what things really should be looking like here going forward.<br></div><div class="gmail_quote"><div><br></div><div>Let me try to make the two proposals so far more concrete.</div><div><br></div><div>Assumptions:</div><div>- We're not going to move CoroSplit early in the pipeline, so this issue really does need to be solved.</div><div>- We should not change the meaning of the C `attribute((const))` to prohibit reading the thread-identity, but, we can change what LLVM IR that transforms into.</div><div>- As a prerequisite, we have already solved the TLS-address-is-a-constant issue -- e.g. by moving TLS address lookup to an intrinsic (I have some other comments about that issue, separately).</div><div><br></div><div>Proposal 1:</div><div>- The readnone attribute is redefined to prohibit depending on thread id.</div><div>- Create a new attribute "readthreadidonly", which is similar to readnone, except that it may access the current thread identity.</div><div>- The new @llvm.threadlocaladdr intrinsic (or whatever it ends up as) is given this attribute.</div><div></div><div>- Clang's `attribute((const))` emits this attribute, instead of readnone.</div><div>- Any other frontends which emit the "readnone" attribute may also need to be updated and emit readthreadidonly instead of readonly.</div><div>- Optimization passes that are oblivious to the new readthreadidonly will fail correct -- they won't <i>misoptimize</i> by moving such a call across an llvm.coro.suspend call (good) -- but they also will regress performance of straightline code (bad).</div><div>- To avoid regressing performance, we'll need to update all optimization passes which currently handle readnone, to also handle readthreadidonly similarly.</div><div>   - Option 1: (quick'n'dirty) treat readthreadidonly as readnone (allowing arbitrary movement) on any function which doesn't contain an llvm.coro.suspend, and treat readthreadidonly as inaccessiblememonly if there are any suspends.</div><div>   - Option 2: actually understand that readthreadid can be moved anywhere except across an llvm.coro.suspend call.</div><div><br></div><div>Proposal 2:</div><div>- The readnone attribute (continues to) allow depending on thread id.</div><div><div>- C frontend's `attribute((const))` continues to emit the readnone attribute.</div><div>- The new @llvm.threadlocaladdr intrinsic is marked readnone.</div><div></div></div><div>- (Optional) add a not_threadid_dependent attribute, which says that a function doesn't care which thread it's invoked on.<br></div><div>- Neither clang nor other frontends need to change which IR attributes they emit.</div><div></div><div>- Optimization passes must be taught not to move readnone calls across llvm.coro.suspend, or they will be incorrect.</div><div>   - Option 1: (quick'n'dirty) treat 'readnone' as 'inaccessiblememonly' in any function which contains a llvm.coro.suspend.</div><div>   - Option 2: actually understand that readnone can move anywhere in the function except across an llvm.coro.suspend call (unless not_threadid_dependent is present, in which case it can also move across that).</div><div></div><div><br></div><div>I'm still slightly in favor of proposal 2, but either seems OK. Both seem like they'll be effectively the same amount of work to implement in LLVM. I feel like having the "not_threadid_dependent"  attribute be separate from "readnone" (doesn't access memory) is logical and consistent -- and having them be distinct may potentially also be useful in other circumstances.</div><div> </div></div></div>