[PATCH] D151774: [LICM] Don't hoist threadlocals within presplit coroutines

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1231
+    // FIXME: We don't handle the semantics of thread local well. So that the
+    // address of thread locals are fake constants in coroutines. So We forbid
+    // to treat onlyReadsMemory call in coroutines as constants now. Note that
----------------
We -> we


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1240
       return true;
     if (Behavior.onlyReadsMemory()) {
       // A readonly argmemonly function only reads from memory pointed to by
----------------
fplk wrote:
> I'm wondering if the new check could be unified with this check to simplify code; they both seem to check for onlyReadsMemory. I didn't double-check yet, but from the naming it seems that onlyReadsMemory and doesNotAccessMemory are mutually exclusive so we can safely move the check to below.
doesNotAccessMemory() also implies onlyReadsMemory(). So I think it's okay to do the separate check that covers both branches.


================
Comment at: llvm/test/Transforms/LICM/sink-with-coroutine.ll:53
+
+define i64 @hoist_threadlocal() presplitcoroutine {
+; CHECK-LABEL: @hoist_threadlocal
----------------
Please precommit the new test together with a run of `update_test_checks.py` on the file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151774/new/

https://reviews.llvm.org/D151774