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 C++20 three-way comparison #485

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

Lancern
Copy link
Member

@Lancern Lancern commented Feb 26, 2024

This patch adds CIRGen support for the C++20 three-way comparison operator <=>. The binary operator is directly lowered to existing CIR operations.

Most of the changes are tests.

@bcardosolopes
Copy link
Member

bcardosolopes commented Mar 1, 2024

Are the check errors related? If so they need to be fixed.

@Lancern
Copy link
Member Author

Lancern commented Mar 1, 2024

Are the check errors related?

After some investigation I found the root cause of the problem. Well, technically this is not a problem of the code, just that the test checks are not general enough.

The problem looks a bit strange at first. For the test input:

auto three_way_strong(int x, int y) {
  return x <=> y;
}

clangir emits different CIR on Linux/Windows and macOS. On Linux or Windows, clangir emits:

%3 = cir.load %0 : cir.ptr <!s32i>, !s32i loc(#loc8)
%4 = cir.load %1 : cir.ptr <!s32i>, !s32i loc(#loc9)
%5 = cir.const(#cir.int<1> : !s8i) : !s8i loc(#loc28)
%6 = cir.const(#cir.int<-1> : !s8i) : !s8i loc(#loc28)
%7 = cir.cmp(lt, %3, %4) : !s32i, !cir.bool loc(#loc28)

On macOS, clangir emits:

%3 = cir.load %0 : cir.ptr <!s32i>, !s32i loc(#loc8)
%4 = cir.load %1 : cir.ptr <!s32i>, !s32i loc(#loc9)
%5 = cir.cmp(lt, %3, %4) : !s32i, !cir.bool loc(#loc28)
%6 = cir.const(#cir.int<-1> : !s8i) : !s8i loc(#loc28)
%7 = cir.const(#cir.int<1> : !s8i) : !s8i loc(#loc28)

Note the order of the last three operations gets reversed on macOS. The test checks only covers the operation sequence on Linux so on macOS the test fails. But the macOS code is still correct and CIRGen code has no problems.

The relevant CIRGen code is the following one-liner at https://github.com/llvm/clangir/pull/485/files#diff-eed2632a5f5feb5d46423a602c86f0b5f5327ef95f2cb43e50bd2c628537461fR1127:

auto SelectOne = EmitSelect(EmitCmp(CK_Less), EmitCmpRes(CmpInfo.getLess()),
                            EmitCmpRes(CmpInfo.getGreater()));

The problem here is that the order of generated CIR operations depends on the evaluation order of the three arguments to the EmitSelect function, which is unspecified behavior. So we can infer on Linux and Windows, they are evaluated from right to left, thus cir.const -> cir.const -> cir.cmp; on macOS they are evaluated from left to right, thus cir.cmp -> cir.const -> cir.const.

So the solution should be updating the test checks so they accept both of the two operation sequence.

@Lancern
Copy link
Member Author

Lancern commented Mar 6, 2024

Unfortunately I could not find a way to make FileCheck accept both output... So I updated the code to make sure that the output CIR operation sequence is consistent across all platforms.

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.

This is awesome, thanks for working on this! Since this is a bit more simple and close to traditional LLVM codegen, I'd prefer if this PR can already raise the three-way cmp functionality:

  • Add a higher level op for the three way cmp and CIRGen that. Should probably be its own Op (as opposed to incorporated into cir.cmp, given we'll need some category information).
  • Emit the code in this PR via LoweringPrepare.

clang/lib/CIR/CodeGen/CIRGenFunction.h Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRGenExprAgg.cpp Outdated Show resolved Hide resolved
@Lancern
Copy link
Member Author

Lancern commented Mar 9, 2024

Add a higher level op for the three way cmp and CIRGen that

Yes this is the longer direction. Since this should be straight-forward enough I'll implement it directly into this PR. So I'm now turning this PR into draft state and will convert it back once this is done.

@Lancern Lancern marked this pull request as draft March 9, 2024 07:39
@Lancern Lancern marked this pull request as ready for review March 10, 2024 03:59
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.

Great, comments inline!

clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIROps.td Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIRAttrs.td Outdated Show resolved Hide resolved
clang/test/CIR/CodeGen/three-way-comparison.cpp Outdated Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIRAttrs.td Outdated Show resolved Hide resolved
clang/include/clang/CIR/Dialect/IR/CIRAttrs.td Outdated Show resolved Hide resolved
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.

LGTM


#include "Inputs/std-compare.h"

// BEFORE: #cmp3way_info_partial_ltn1eq0gt1unn127_ = #cir.cmp3way_info<partial, lt = -1, eq = 0, gt = 1, unordered = -127>
Copy link
Member

Choose a reason for hiding this comment

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

Mixed feelings because it's nice to have all that info, but maybe it's too big? Should we get a bit more terse in the alias? I was thinking #cmp3HASH at most. Anyways it's something to be addressed in another PR, this is good for now.

@bcardosolopes
Copy link
Member

Some conflicts need to be address before merge

This patch adds CIRGen support for the C++20 three-way comparison operator
`<=>`. The binary operator is directly lowered to existing CIR operations.
@Lancern Lancern force-pushed the three-way-comparison branch from 361d62c to a47de98 Compare March 18, 2024 14:03
@Lancern
Copy link
Member Author

Lancern commented Mar 18, 2024

Squashed and rebased onto the latest main.

@bcardosolopes bcardosolopes merged commit 43551d4 into llvm:main Mar 19, 2024
6 checks passed
@Lancern Lancern deleted the three-way-comparison branch March 19, 2024 01:33
@Lancern
Copy link
Member Author

Lancern commented Mar 19, 2024

As a side note for potential future improvements: LLVM now has an RFC that adds 3-way comparison intrinsic. LLVM folks believe this new intrinsic can generate better code for various targets. This project will be done in GSoC2024 and when it's done we can consider lowering cir.cmp3way to it directly.

lanza pushed a commit that referenced this pull request Mar 23, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Apr 29, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
bruteforceboy pushed a commit to bruteforceboy/clangir that referenced this pull request Oct 2, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
Hugobros3 pushed a commit to shady-gang/clangir that referenced this pull request Oct 2, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
lanza pushed a commit that referenced this pull request Nov 5, 2024
This patch adds CIRGen support for the C++20 three-way comparison
operator `<=>`. The binary operator is directly lowered to existing CIR
operations.

Most of the changes are tests.
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.

2 participants