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.

Sign up for GitHub

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

Jump to bottom

[X86] _mm_test_all_ones - repeated use of macro argument #60006

Closed
RKSimon opened this issue Jan 13, 2023 · 7 comments · Fixed by llvm/llvm-project-release-prs#231
Closed

[X86] _mm_test_all_ones - repeated use of macro argument #60006

RKSimon opened this issue Jan 13, 2023 · 7 comments · Fixed by llvm/llvm-project-release-prs#231
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics release:backport release:merged
Milestone
LLVM 16.0.0 Release

Comments

@RKSimon
Copy link
Collaborator

RKSimon commented Jan 13, 2023 •
edited by VoltrexKeyva 

#define _mm_test_all_ones(val) _mm_testc_si128((val), _mm_cmpeq_epi32((val),(val)))

val may be re-evaluated due to its additional uses

#include <x86intrin.h>

__m128i expensive_call();
bool alltrue() {
    return _mm_test_all_ones(expensive_call());
}
alltrue():                            # @alltrue()
        subq    $40, %rsp
        callq   expensive_call()@PLT
        vmovdqa %xmm0, 16(%rsp)                 # 16-byte Spill
        callq   expensive_call()@PLT
        vmovdqa %xmm0, (%rsp)                   # 16-byte Spill
        callq   expensive_call()@PLT
        vpcmpeqd        (%rsp), %xmm0, %xmm0            # 16-byte Folded Reload
        vmovdqa 16(%rsp), %xmm1                 # 16-byte Reload
        vptest  %xmm0, %xmm1
        setb    %al
        addq    $40, %rsp
        retq
@RKSimon RKSimon added backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Jan 13, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 13, 2023

@llvm/issue-subscribers-backend-x86

@phoebewang
Copy link
Contributor

Agreed. This is indeed a problem. Maybe change it to function?

@RKSimon
Copy link
Collaborator Author

RKSimon commented Jan 13, 2023 •
edited by VoltrexKeyva

Either that or define it as:

#define _mm_test_all_ones(val) _mm_testc_si128((val), _mm_set1_epi32(-1))

@RKSimon
Copy link
Collaborator Author

RKSimon commented Jan 24, 2023

Candidate Patch: https://reviews.llvm.org/D142477

@RKSimon RKSimon reopened this Jan 25, 2023
@RKSimon RKSimon added this to the LLVM 16.0.0 Release milestone Jan 25, 2023
@RKSimon
Copy link
Collaborator Author

RKSimon commented Jan 25, 2023

/cherry-pick c9b2823

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 25, 2023

/branch llvm/llvm-project-release-prs/issue60006

@llvmbot llvmbot mentioned this issue Jan 25, 2023
Merged
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 25, 2023

/pull-request llvm/llvm-project-release-prs#231

tru pushed a commit that referenced this issue Jan 27, 2023
@RKSimon @tru
[X86] Ensure the _mm_test_all_ones macro does not reuse argument (PR6…
99777ef 
…0006)

The macro _mm_test_all_ones(V) was defined as _mm_testc_si128((V), _mm_cmpeq_epi32((V), (V))) - which could cause side effects depending on the source of the V value.

The _mm_cmpeq_epi32((V), (V)) trick was just to materialize an all-ones value, which can be more safely generated with _mm_set1_epi32(-1) .

Fixes #60006

Differential Revision: https://reviews.llvm.org/D142477

(cherry picked from commit c9b2823)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics release:backport release:merged
Projects
LLVM Release Status
Archived in project
Milestone
LLVM 16.0.0 Release
Development

Successfully merging a pull request may close this issue.

PR for llvm/llvm-project#60006 llvm/llvm-project-release-prs
4 participants
@RKSimon @phoebewang @EugeneZelenko @llvmbot