Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR][CIRGen] Support for __builtin_expect #478

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

YazZz1k
Copy link
Contributor

@YazZz1k YazZz1k commented Feb 21, 2024

This PR adds the new cir.expect opcode which is similar to llvm.expect intricnsics.
Codegen of __builtin_expect emits cir.expect opcode. Then cir.expect will be lowered to llvm.expect intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of if often emits the lllvm IR with redundant cast instructions. Like this:

%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2

But the llvm pass LowerExpectIntrinsicPass (that should replace llvm.expect with branch metadata) performs only simple pattern-matching. And it can't handle this zext/trunc intructions. So this pass in such cases just removes the llvm.expect without updating a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast instruction sequence.

@YazZz1k YazZz1k force-pushed the builtin-expect branch 2 times, most recently from d7c6c42 to 6f559dc Compare February 21, 2024 14:39
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Mostly good, few small comments to address.

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp Outdated Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved
@Lancern
Copy link
Member

Lancern commented Feb 22, 2024

I believe the root problem here is that CIR lowers !cir.bool in a different way than the original clang CodeGen.

In the original CodeGen, bool glvalues are lowered to LLVM i8 values, and bool prvalues are lowered to LLVM i1 values, as illustrated in the following example:

bool test(int x) {
  bool ret = x > 42;
  return ret;
}
; @test returns a bool prvalue so its return type is `i1`
define dso_local noundef zeroext i1 @test()() {
entry:
  %ret = alloca i8, align 1  ; %ret is a bool glvalue so its type is `i8`
  tail call void @llvm.dbg.declare(metadata ptr %ret, metadata !16, metadata !DIExpression())
  store i8 1, ptr %ret, align 1
  %0 = load i8, ptr %ret, align 1
  %tobool = trunc i8 %0 to i1
  ret i1 %tobool
}

However, in CIRGen, all !cir.bool values are lowered to LLVM i8 values. The example above would be lowered to LLVMIR through CIR as:

; Note that the return value of @test is `i8` rather than `i1`
define i8 @test()() #0 !dbg !3 {
  %1 = alloca i8, i64 1, align 1, !dbg !6
  %2 = alloca i8, i64 1, align 1, !dbg !7
  store i8 1, ptr %2, align 1, !dbg !7
  %3 = load i8, ptr %2, align 1, !dbg !8
  store i8 %3, ptr %1, align 1, !dbg !9
  %4 = load i8, ptr %1, align 1, !dbg !9
  ret i8 %4, !dbg !9
}

This divergence leads to the redundancy illustrated in the PR description. The result of a cir.cmp operation is currently expected to be lowered to an i8 value although it's a prvalue. After emitting an llvm.icmp operation, you have to insert an llvm.zext operation to extend the i1 to i8. Thus the redundancy.

@bcardosolopes
Copy link
Member

I believe the root problem here is that CIR lowers !cir.bool in a different way than the original clang CodeGen.

Yes, this is a known issue, but looks like we don't have a github one tracking it, let me create it. Thanks for making it a detailed description, very useful.

Btw, if this is about my comment on adding more info, I just want a better explanation in the comments so there's some context on how to fix it in the future - it doesn't need to be as complete as the awesome one you just gave though.

Copy link

github-actions bot commented Feb 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@YazZz1k YazZz1k force-pushed the builtin-expect branch 4 times, most recently from a7f19c9 to e571cea Compare February 29, 2024 12:06
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good, few nitpicks + conflicts need to be fixed.

clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved
clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp Outdated Show resolved Hide resolved
@YazZz1k YazZz1k force-pushed the builtin-expect branch 2 times, most recently from ea0b2a3 to be7b432 Compare March 1, 2024 15:32
@bcardosolopes bcardosolopes merged commit 7c59e01 into llvm:main Mar 5, 2024
6 checks passed
lanza pushed a commit that referenced this pull request Mar 23, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Apr 29, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
This PR adds the new `cir.expect` opcode which is similar to llvm.expect
intricnsics.
Codegen of `__builtin_expect` emits `cir.expect` opcode. Then
`cir.expect` will be lowered to `llvm.expect` intrinsic.

When implementing __builtin_expect I faced with minor issue.
CIR lowering of `if` often emits the lllvm IR with redundant cast
instructions. Like this:
```
%0 = call i64 @llvm.expect.i64(i64 %any, i64 1), !dbg !13
%1 = icmp ne i64 %0, 0
%2 = zext i1 %0 to i8   // redundant
%3 = trunc i8 %1 to i1 // redundant
br i1 %3, label %l1, label %l2
```
But the llvm pass `LowerExpectIntrinsicPass` (that should replace
`llvm.expect` with branch metadata) performs only simple
pattern-matching. And it can't handle this zext/trunc intructions. So
this pass in such cases just removes the `llvm.expect` without updating
a branch metadata.
In this reason this PR also avoid emitting the redundant zext/cast
instruction sequence.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants