This patch accompanies the RFC posted here:
http://lists.llvm.org/pipermail/llvm-dev/2018-October/127239.html
This is the clang side. The llvm version is in D53765
Differential D56571
[RFC prototype] Implementation of asm-goto support in clang jyu2 on Jan 10 2019, 5:25 PM. Authored by Tags None Tokens
Details This patch accompanies the RFC posted here: This is the clang side. The llvm version is in D53765
Diff Detail Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions Remove check for %lN
Comment Actions I find some ambiguous error for asm-goto out of scope. Let me know if you see any problems. Thanks for all the review. Comment Actions I think this is in good shape.
Comment Actions Review comment addressed
Comment Actions Should the langref be updated (specifically the section on blockaddress): This value only has defined behavior when used as an operand to the ‘indirectbr’ instruction, or for comparisons against null. https://reviews.llvm.org/D53765 touched the langref, but I think the blockaddress section can be cleaned up. Comment Actions @rsmith, @efriedma, @chandlerc, is this in good enough shape that we can commit and move to incremental fixes/improvement? Jennifer has had to rebase this a couple times which is making things hard for the folks testing this with the Linux kernel and needing to apply this patch themselves. Comment Actions I'm fine merging in this state... but please wait for rsmith to respond before you merge. Comment Actions
If we can get inlining of callbr working first before landing this, then all of our CI won't go immediately red (as it would from landing this first, then getting inlining support). Otherwise, we'll have to come up with hacks in the kernel to keep the CI green (I have one for x86. The one for aarch64 quickly became infeasible). I'm anxious for this to land, and the rebases are work indeed. But if we can just wait a little longer and get inlining support working, then land this, it will be much less painful than the other way around. I'm focused on inlining support in https://reviews.llvm.org/D58260 (which doesn't look like much now, but I have changes locally and have been actively working on it this week). Comment Actions Note that with:
inline-asm according to getBooleanType")
blockaddresses if sole uses are callbrs")
asm-goto support in clang") I can compile a mainline x86 defconfig Linux kernel and boot it in QEMU. Comment Actions This code: ; ModuleID = 'arch_static_branch.bc' source_filename = "arch/x86/entry/vsyscall/vsyscall_64.c" target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-grtev4-linux-gnu" %struct.atomic_t = type { i32 } %struct.jump_entry = type { i64, i64, i64 } %struct.tracepoint_func = type { i8*, i8*, i32 } %struct.static_key = type { %struct.atomic_t, %union.anon } %union.anon = type { i64 } @__tracepoint_emulate_vsyscall = external hidden global { i8*, { %struct.atomic_t, { %struct.jump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }, section "__tracepoints", align 8 ; Function Attrs: alwaysinline noredzone nounwind sspstrong define zeroext i1 @arch_static_branch(%struct.static_key* nocapture readnone %key, i1 zeroext %branch) { entry: callbr void asm sideeffect "1:.byte 0x0f,0x1f,0x44,0x00,0\0A\09.pushsection __jump_table, \22aw\22 \0A\09 .balign 8 \0A\09 .quad 1b, ${2:l}, ${0:c} + ${1:c} \0A\09.popsection \0A\09", "i,i,X,~{dirflag},~{fpsr},~{flags}"(%struct.static_key* bitcast (i32* getelementptr inbounds ({ i8*, { %struct.atomic_t, { %struct.jump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }, { i8*, { %struct.atomic_t, { %struct.j ump_entry* } }, i32 ()*, void ()*, %struct.tracepoint_func* }* @__tracepoint_emulate_vsyscall, i64 0, i32 1, i32 0, i32 0) to %struct.static_key*), i1 false, i8* blockaddress(@arch_static_branch, %return)) to label %asm.fallthrough [label %return] asm.fallthrough: ; preds = %entry call void asm sideeffect "", "~{dirflag},~{fpsr},~{flags}"() br label %return return: ; preds = %asm.fallthrough, %entry %retval.0 = phi i1 [ false, %asm.fallthrough ], [ true, %entry ] ret i1 %retval.0 } gives me this with the asm-goto patches: $ clang -o /dev/null arch_static_branch.ll warning: overriding the module target triple with x86_64-unknown-linux-gnu [-Woverride-module] <inline asm>:4:15: error: expected a symbol reference in '.quad' directive .quad 1b, "", __tracepoint_emulate_vsyscall+8 + 0 ^ error: cannot compile inline asm 1 warning and 1 error generated. Comment Actions I think things don't work right unless you disable the integrated assembler. I'm not sure why though. Comment Actions Using -no-integrated-as does allow it to compile, but it doesn't link (with ld.lld) if LTO is specified. Is there an equivalent flag / plugin-opt for lld? Comment Actions There shouldn't be an empty string ("") in the asm output -- that should be a reference to the "l_yes" label, not the empty string. That seems very weird... Even odder: running clang -S on the above without -fno-integrated-as emits a ".quad .Ltmp00" (note the extra "0"!), while with -fno-integrated-as, it properly refers to ".Ltmp0". Comment Actions The changes in ASTImporter.cpp looks good to me! Comment Actions Hi, First, thanks for working on this. I am the author of the "rseq" system call in the Linux kernel, and the user-space code required to interact with that system call requires asm goto. I therefore look forward to getting asm goto support in clang. I tried this patch on top of current clang on the following Linux kernel rseq selftests, and it fails to build. Hopefully this feedback can help you improve the current implementation. The kernel rseq selftests can be found at: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/rseq from the kernel source tree (on x86-64): make headers_install cd tools/testing/selftests/rseq make CC=/path/to/clang build output: /home/efficios/git/llvm-project/build/bin/clang -O2 -Wall -g -I./ -I../../../../usr/include/ -L./ -Wl,-rpath=./ -shared -fPIC rseq.c -lpthread -o /home/efficios/git/linux/tools/testing/selftests/rseq/librseq.so /home/efficios/git/llvm-project/build/bin/clang -O2 -Wall -g -I./ -I../../../../usr/include/ -L./ -Wl,-rpath=./ basic_test.c -lpthread -lrseq -o /home/efficios/git/linux/tools/testing/selftests/rseq/basic_test /home/efficios/git/llvm-project/build/bin/clang -O2 -Wall -g -I./ -I../../../../usr/include/ -L./ -Wl,-rpath=./ basic_percpu_ops_test.c -lpthread -lrseq -o /home/efficios/git/linux/tools/testing/selftests/rseq/basic_percpu_ops_test In file included from basic_percpu_ops_test.c:12: In file included from ./rseq.h:71: ./rseq-x86.h:90:26: error: expected a symbol reference "cmpq %[v], %[expect]\n\t" ^ <inline asm>:13:8: note: instantiated into assembly here jnz "" ^ In file included from basic_percpu_ops_test.c:12: In file included from ./rseq.h:71: ./rseq-x86.h:102:3: error: expected a symbol reference RSEQ_ASM_DEFINE_ABORT(4, "", abort) ^ ./rseq-x86.h:67:25: note: expanded from macro 'RSEQ_ASM_DEFINE_ABORT' __rseq_str(label) ":\n\t" \ ^ <inline asm>:20:8: note: instantiated into assembly here jmp "" ^ In file included from basic_percpu_ops_test.c:12: In file included from ./rseq.h:71: ./rseq-x86.h:148:30: error: expected a symbol reference "cmpq %%rbx, %[expectnot]\n\t" ^ <inline asm>:14:7: note: instantiated into assembly here je "" ^ In file included from basic_percpu_ops_test.c:12: In file included from ./rseq.h:71: ./rseq-x86.h:164:3: error: expected a symbol reference RSEQ_ASM_DEFINE_ABORT(4, "", abort) ^ ./rseq-x86.h:67:25: note: expanded from macro 'RSEQ_ASM_DEFINE_ABORT' __rseq_str(label) ":\n\t" \ ^ <inline asm>:24:8: note: instantiated into assembly here jmp "" ^ 4 errors generated. Makefile:22: recipe for target '/home/efficios/git/linux/tools/testing/selftests/rseq/basic_percpu_ops_test' failed make: *** [/home/efficios/git/linux/tools/testing/selftests/rseq/basic_percpu_ops_test] Error 1 Comment Actions @compudj email me the preprocessed output of basic_percpu_ops_test.c and I'll take a look. (Should be able to find my email via git log). Comment Actions @nickdesaulniers, thanks for looking into this. It look like error come from: llvm/lib/MC/MCParser/AsmParser.cpp:1118. Please let me know if that is clang problem. Thanks. Comment Actions It's an orthogonal issue with Clang's integrated assembler. @compudj confirmed that setting -no-integrated-as solves the issue (which is what we do throughout the kernel, except in a few places mostly by accident). I'm following up with @compudj off thread. If we can get help w/ code review of: Then I think we'll have everything we need to land this patch, for the Linux kernel's consumption. Comment Actions Ok, from the Linux kernel's perspective, I believe we have worked out all underlying issues in LLVM wrt callbr that would prevent us from using asm goto successfully. This patch now has my blessing; thanks for all the hard work that went into this large feature. Please wait for a final LGTM from @rsmith . Comment Actions Looks good with a few largely-mechanical changes.
Comment Actions Was this committed accidentally? Today in master I see:
and yet this Phabricator review is still open. It may be easier to rebase this into a monorepo checkout, then commit with git llvm push. As @kees noted, for new test files that contain x86 assembly, they need a -triple that specifies an x86 target, otherwise the implicit target is based on the host, which may not be x86. We still want to verify that non-x86 host can cross compile to x86 correctly. Sorry we did not catch this earlier in code review. Comment Actions jyu2 committed rG954ec09aed4f: clang support gnu asm goto Thanks Kees and Nick! I am currently work on those tests to see if I can make them more generic. All my test using x86 instruction. So that is not working for arm or arm64 target. Will update you once I am done for this. Comment Actions jyu2 committed rGb8fee677bf8e: Re-check in clang support gun asm goto after fixing tests. (authored by jyu2). Comment Actions There is a bug.. I took GCC’s example for asm goto and trunk emits: Wrong jb instruction target.. Comment Actions I think Jennifer just forgot the Differential Revision line, so this was never closed. The test failures resulted in her wanting the patch reverted until she could fix it (hence my revert) Hmm... I don't see the jb instruction target name reflected in the IR, so I suspect @craig.topper should be made aware of the descrepancy (author of https://reviews.llvm.org/D53765). Comment Actions There's still something weird in the backend, but things seem to generally work if you pass -fno-integrated-as to clang which the linux kernel does. Comment Actions For future travelers, we're tracking 2 related bugs:
|