[RFC] __builtin_constant_p() Improvements

Project Infrastructure LLVM Dev List Archives
1

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

bcp.patch (3.93 KB)

2

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?

3

I actually was working on an updated patch for the LLVM-side of this, also. :slight_smile: 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.

4

I actually was working on an updated patch for the LLVM-side of this, also. :slight_smile: 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. :slight_smile:

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