<div dir="ltr">I kind of like proposal 2, option 1, quick'n'dirty. Consider that Instruction::mayReadFromMemory could be taught to check if its parent function is a coroutine, and power down in those cases. Then it's a matter of ensuring that all code motion passes use the central utility.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Dec 10, 2020 at 12:01 PM James Y Knight via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>