Page MenuHomePhabricator

[RFC] Fix TLS and Coroutine
Changes PlannedPublic

Authored by lxfind on Dec 4 2020, 8:47 AM.

Details

Summary

This patch is to address https://bugs.llvm.org/show_bug.cgi?id=47835.
A relevant discussion regarding pthread_self and TLS can be found here: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146766.html.

A coroutine may suspend and resume on a different thread, and hence the address of a thread_local variable may change after coroutine suspension.
In the existing design, getting the address of a TLS variable is through a direct reference, like @tls_variable. Such kind of value can be
arbitrarily moved around/replaced in the IR within the same function. This will lead to incorrect caching of TLS variable address in coroutines across suspension points.
To fix it, we have to turn the TLS address access into an intrinsics call, so that it will not be simply CSE-ed.
After CoroSplit, we no longer have coroutines, and hence can safely lower the TLS intrinsics back into references.

Note:
The current placement of the LowerThreadLocalIntrinsicPass may not be ideal. I am not quite sure how to organize it. Suggestions welcome!
Testing isn't sufficient, and there may also be failing tests. I will add/fix more tests if this patch is along the right direction.

Diff Detail

Unit TestsFailed

TimeTest
320 msx64 debian > Clang.CodeGen::no-skipped-passes-O0-opt-bisect.c
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-linux-gnu -O0 -fexperimental-new-pass-manager /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/no-skipped-passes-O0-opt-bisect.c -fdebug-pass-manager -emit-llvm -o /dev/null 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/no-skipped-passes-O0-opt-bisect.c
110 msx64 debian > Clang.CodeGenCoroutines::coro-tls.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -fcoroutines-ts -std=c++14 -O3 -emit-llvm -S /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCoroutines/coro-tls.cpp -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGenCoroutines/coro-tls.cpp
140 msx64 debian > LLVM.CodeGen/AMDGPU::opt-pipeline.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt -O0 -mtriple=amdgcn--amdhsa -disable-output -disable-verify -debug-pass=Structure /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AMDGPU/opt-pipeline.ll 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck -check-prefix=GCN-O0 /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
50 msx64 debian > LLVM.Other::opt-O2-pipeline.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt -enable-new-pm=0 -mtriple=x86_64-- -O2 -debug-pass=Structure < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Other/opt-O2-pipeline.ll -o /dev/null 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --check-prefixes=CHECK,CHECK-NOEXT /mnt/disks/ssd0/agent/llvm-project/llvm/test/Other/opt-O2-pipeline.ll
50 msx64 debian > LLVM.Other::opt-O3-pipeline-enable-matrix.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt -enable-new-pm=0 -O3 -enable-matrix -debug-pass=Structure < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Other/opt-O3-pipeline-enable-matrix.ll -o /dev/null 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Other/opt-O3-pipeline-enable-matrix.ll
View Full Test Results (16 Failed)

Event Timeline

lxfind created this revision.Dec 4 2020, 8:47 AM
lxfind requested review of this revision.Dec 4 2020, 8:47 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
hoy added inline comments.Dec 4 2020, 9:28 AM
llvm/include/llvm/IR/Intrinsics.td
1309

With the intrinsic, can TLS variable reference in the same coroutine or regular routine be DCE-ed anymore?

hoy added inline comments.Dec 4 2020, 9:31 AM
llvm/include/llvm/IR/Intrinsics.td
1309

Sorry, I meant CSE-ed.

lxfind added inline comments.Dec 4 2020, 10:08 AM
llvm/include/llvm/IR/Intrinsics.td
1309

Since the intrinsics does not have readnone attribute, it won't be CSE-ed before CoroSplit.
However after CoroSplit, it will be lowered back to the direct reference of the TLS, and will be CSE-ed by latter passes.
I can add a test function to demonstrate that too.

hoy added inline comments.Dec 4 2020, 1:38 PM
llvm/include/llvm/IR/Intrinsics.td
1309

Sounds good. Can you please point out what optimization passes CSE-ed tls reference without this implementation? I'm wondering if those optimizations can be postponed to after CoroSplit.

lxfind added inline comments.Dec 4 2020, 3:01 PM
llvm/include/llvm/IR/Intrinsics.td
1309

To clarify, it wasn't just CSE that would merge the references of the same TLS.
For instance, without this patch, a reference to "tls_variable" will just be "@tls_variable". For code like this:

@tls_variable = internal thread_local global i32 0, align 4

define i32* @foo(){
  ret i32* @tls_variable
}

define void @bar() {
  %tls1 = call i32* @foo()
  ..coro.suspend..
  %tls2 = call i32* @foo()
  %cond = icmp eq i32* %tls1, %tls2
}

When foo() is inlined into bar(), all uses of %tls1 will be replaced with @tls_variable.

hoy added inline comments.Dec 7 2020, 11:16 PM
llvm/include/llvm/IR/Intrinsics.td
1309

Thanks for the explanation. I have a dumb question. Why isn't corosplit placed at the very beginning of the pipeline?

lxfind added inline comments.Dec 8 2020, 8:27 AM
llvm/include/llvm/IR/Intrinsics.td
1309

The coroutine frame size is determined during CoroSplit. So if CoroSplit happens too early without any optimizations, the frame size will always be very big and there is no chance to optimize it.
This is indeed a fundamental trade-off. If CoroSplit happens much earlier then it will be immune to this kind of problem.

nhaehnle added inline comments.
llvm/include/llvm/IR/Intrinsics.td
1281–1282

Unrelated change

lxfind updated this revision to Diff 310575.Dec 9 2020, 10:18 AM

Fix all failing tests

lxfind planned changes to this revision.Feb 18 2021, 10:36 AM