Bill Wendling via llvm-dev
2018-Apr-12 21:09 UTC
[llvm-dev] [RFC] __builtin_constant_p() Improvements
Hello again!
I took a stab at PR4898[1]. The attached patch improves Clang's
__builtin_constant_p support so that the Linux kernel is happy. With this
improvement, Clang can determine if __builtin_constant_p is true or false
after inlining.
As an example:
static __attribute__((always_inline)) int foo(int x) {
if (__builtin_constant_p(x))
return 1;
return 0;
}
static __attribute__((always_inline)) int mux() {
if (__builtin_constant_p(37))
return 927;
return 0;
}
int bar(int a) {
if (a)
return foo(42);
else
return mux();
}
Now outputs this code at -O1:
bar:
.cfi_startproc
# %bb.0: # %entry
testl %edi, %edi
movl $927, %ecx # imm = 0x39F
movl $1, %eax
cmovel %ecx, %eax
retq
And this code at -O0:
bar: # @bar
.cfi_startproc
# %bb.0: # %entry
pushq %rbp
.cfi_def_cfa_offset 16
.cfi_offset %rbp, -16
movq %rsp, %rbp
.cfi_def_cfa_register %rbp
movl %edi, -16(%rbp)
cmpl $0, -16(%rbp)
je .LBB0_2
# %bb.1: # %if.then
movl $42, -8(%rbp)
movl $0, -4(%rbp)
movl -4(%rbp), %eax
movl %eax, -12(%rbp)
jmp .LBB0_3
.LBB0_2: # %if.else
movl $927, -12(%rbp) # imm = 0x39F
.LBB0_3: # %return
movl -12(%rbp), %eax
popq %rbp
retq
If the patch looks okay to people, I can shove it onto Phabricator for a
review. (My phab-fu is bad.)
Thoughts?
-bw
[1] https://bugs.llvm.org/show_bug.cgi?id=4898
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180412/8a2d3328/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bcp.patch
Type: text/x-patch
Size: 4024 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180412/8a2d3328/attachment.bin>
Saleem Abdulrasool via llvm-dev
2018-Apr-13 05:56 UTC
[llvm-dev] [RFC] __builtin_constant_p() Improvements
On Thu, Apr 12, 2018 at 2:09 PM, Bill Wendling via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hello again! > > I took a stab at PR4898[1]. The attached patch improves Clang's > __builtin_constant_p support so that the Linux kernel is happy. With this > improvement, Clang can determine if __builtin_constant_p is true or false > after inlining. > > As an example: > > static __attribute__((always_inline)) int foo(int x) { > if (__builtin_constant_p(x)) > return 1; > return 0; > } > > static __attribute__((always_inline)) int mux() { > if (__builtin_constant_p(37)) > return 927; > return 0; > } > > int bar(int a) { > if (a) > return foo(42); > else > return mux(); > } > > Now outputs this code at -O1: > > bar: > .cfi_startproc > # %bb.0: # %entry > testl %edi, %edi > movl $927, %ecx # imm = 0x39F > movl $1, %eax > cmovel %ecx, %eax > retq > > And this code at -O0: > > bar: # @bar > .cfi_startproc > # %bb.0: # %entry > pushq %rbp > .cfi_def_cfa_offset 16 > .cfi_offset %rbp, -16 > movq %rsp, %rbp > .cfi_def_cfa_register %rbp > movl %edi, -16(%rbp) > cmpl $0, -16(%rbp) > je .LBB0_2 > # %bb.1: # %if.then > movl $42, -8(%rbp) > movl $0, -4(%rbp) > movl -4(%rbp), %eax > movl %eax, -12(%rbp) > jmp .LBB0_3 > .LBB0_2: # %if.else > movl $927, -12(%rbp) # imm = 0x39F > .LBB0_3: # %return > movl -12(%rbp), %eax > popq %rbp > retq > > If the patch looks okay to people, I can shove it onto Phabricator for a > review. (My phab-fu is bad.) > > Thoughts? >The patch seems reasonable. Could you please also add docs on the intrinsic itself?> -bw > > [1] https://bugs.llvm.org/show_bug.cgi?id=4898 > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > >-- Saleem Abdulrasool compnerd (at) compnerd (dot) org -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180412/949ed222/attachment.html>
James Y Knight via llvm-dev
2018-Apr-13 16:12 UTC
[llvm-dev] [RFC] __builtin_constant_p() Improvements
I actually was working on an updated patch for the LLVM-side of this, also. :) I was just working on some test cases; I'll post it soon. It's somewhat different than yours. I haven't touched the clang side yet, but I think it needs to be more complex than what you have there. I think it actually needs to be able to evaluate the intrinsic as a constant _false_ in the front-end in some circumstances, rather than always emitting IR if it cannot tell the answer is true. On Thu, Apr 12, 2018 at 5:10 PM Bill Wendling via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hello again! > > I took a stab at PR4898[1]. The attached patch improves Clang's > __builtin_constant_p support so that the Linux kernel is happy. With this > improvement, Clang can determine if __builtin_constant_p is true or false > after inlining. > > As an example: > > static __attribute__((always_inline)) int foo(int x) { > if (__builtin_constant_p(x)) > return 1; > return 0; > } > > static __attribute__((always_inline)) int mux() { > if (__builtin_constant_p(37)) > return 927; > return 0; > } > > int bar(int a) { > if (a) > return foo(42); > else > return mux(); > } > > Now outputs this code at -O1: > > bar: > .cfi_startproc > # %bb.0: # %entry > testl %edi, %edi > movl $927, %ecx # imm = 0x39F > movl $1, %eax > cmovel %ecx, %eax > retq > > And this code at -O0: > > bar: # @bar > .cfi_startproc > # %bb.0: # %entry > pushq %rbp > .cfi_def_cfa_offset 16 > .cfi_offset %rbp, -16 > movq %rsp, %rbp > .cfi_def_cfa_register %rbp > movl %edi, -16(%rbp) > cmpl $0, -16(%rbp) > je .LBB0_2 > # %bb.1: # %if.then > movl $42, -8(%rbp) > movl $0, -4(%rbp) > movl -4(%rbp), %eax > movl %eax, -12(%rbp) > jmp .LBB0_3 > .LBB0_2: # %if.else > movl $927, -12(%rbp) # imm = 0x39F > .LBB0_3: # %return > movl -12(%rbp), %eax > popq %rbp > retq > > If the patch looks okay to people, I can shove it onto Phabricator for a > review. (My phab-fu is bad.) > > Thoughts? > > -bw > > [1] https://bugs.llvm.org/show_bug.cgi?id=4898 > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180413/0db507a7/attachment.html>
Bill Wendling via llvm-dev
2018-Apr-13 19:58 UTC
[llvm-dev] [RFC] __builtin_constant_p() Improvements
On Fri, Apr 13, 2018 at 9:12 AM James Y Knight <jyknight at google.com> wrote:> I actually was working on an updated patch for the LLVM-side of this, > also. :) I was just working on some test cases; I'll post it soon. It's > somewhat different than yours. > > Oh cool! I'll look at the Phab entry. :-)> I haven't touched the clang side yet, but I think it needs to be more > complex than what you have there. I think it actually needs to be able to > evaluate the intrinsic as a constant _false_ in the front-end in some > circumstances, rather than always emitting IR if it cannot tell the answer > is true. > > It would only be "false" if the function cannot be inlined, right?Otherwise you don't really know. So at -O0, we can safely mark something as "false". But in other cases, we would need to be wary. -bw> On Thu, Apr 12, 2018 at 5:10 PM Bill Wendling via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Hello again! >> >> I took a stab at PR4898[1]. The attached patch improves Clang's >> __builtin_constant_p support so that the Linux kernel is happy. With >> this improvement, Clang can determine if __builtin_constant_p is true or >> false after inlining. >> >> As an example: >> >> static __attribute__((always_inline)) int foo(int x) { >> if (__builtin_constant_p(x)) >> return 1; >> return 0; >> } >> >> static __attribute__((always_inline)) int mux() { >> if (__builtin_constant_p(37)) >> return 927; >> return 0; >> } >> >> int bar(int a) { >> if (a) >> return foo(42); >> else >> return mux(); >> } >> >> Now outputs this code at -O1: >> >> bar: >> .cfi_startproc >> # %bb.0: # %entry >> testl %edi, %edi >> movl $927, %ecx # imm = 0x39F >> movl $1, %eax >> cmovel %ecx, %eax >> retq >> >> And this code at -O0: >> >> bar: # @bar >> .cfi_startproc >> # %bb.0: # %entry >> pushq %rbp >> .cfi_def_cfa_offset 16 >> .cfi_offset %rbp, -16 >> movq %rsp, %rbp >> .cfi_def_cfa_register %rbp >> movl %edi, -16(%rbp) >> cmpl $0, -16(%rbp) >> je .LBB0_2 >> # %bb.1: # %if.then >> movl $42, -8(%rbp) >> movl $0, -4(%rbp) >> movl -4(%rbp), %eax >> movl %eax, -12(%rbp) >> jmp .LBB0_3 >> .LBB0_2: # %if.else >> movl $927, -12(%rbp) # imm = 0x39F >> .LBB0_3: # %return >> movl -12(%rbp), %eax >> popq %rbp >> retq >> >> If the patch looks okay to people, I can shove it onto Phabricator for a >> review. (My phab-fu is bad.) >> >> Thoughts? >> >> -bw >> >> [1] https://bugs.llvm.org/show_bug.cgi?id=4898 >> > _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180413/46a9133d/attachment.html>