Skip to content

Conversation

wizardengineer
Copy link
Contributor

Related Issue: #1880

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

Needs some documentation in the codegen handling, and a couple of nits. There are a lot of tests and I don't see how all of them are necessary. Some are also underspecified to the point where they don't check what they claim to check.

Comment on lines 55 to 67
// CHECK-LABEL: @_Z23test_pslldqi128_shift20Dv2_x
__m128i test_pslldqi128_shift20(__m128i a) {
// Should also return zero
// CHECK: %{{.*}} = cir.const #cir.zero : !cir.vector<!s64i x 2>
return __builtin_ia32_pslldqi128_byteshift(a, 20);
}

// CHECK-LABEL: @_Z28test_pslldqi128_masked_shiftDv2_x
__m128i test_pslldqi128_masked_shift(__m128i a) {
// 250 > 16, so should return zero
// CHECK: %{{.*}} = cir.const #cir.zero : !cir.vector<!s64i x 2>
return __builtin_ia32_pslldqi128_byteshift(a, 250);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we need both of these tests?

Comment on lines 1 to 2
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir -target-feature +avx512f
// RUN: FileCheck --input-file=%t.cir %s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add cir-to-llvm (LLVM) and OG codegen (OGCG) tests?

@tommymcm
Copy link
Collaborator

tommymcm commented Sep 2, 2025

Closes #1880

@xlauko
Copy link
Collaborator

xlauko commented Sep 4, 2025

lgtm, after @tommymcm comments are resolved

// LLVM: store <2 x i64> zeroinitializer, ptr %{{.*}}, align 16
// OGCG: ret <2 x i64> zeroinitializer
return __builtin_ia32_pslldqi128_byteshift(a, 240);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should still keep this test? I do agree the tests previously was redundant, but I only did so for the sake of stress testing my implementation in order to make sure we catch the issues now rather than later.

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

lgtm

@bcardosolopes bcardosolopes merged commit 327ce45 into llvm:main Sep 5, 2025
8 of 9 checks passed
tommymcm pushed a commit to tommymcm/clangir that referenced this pull request Sep 5, 2025
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.

4 participants