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

[InstSimplify] (a ^ b) ? (~a) ^ b : a ^ (~b) #63104

Closed
k-arrows opened this issue Jun 4, 2023 · 7 comments
Closed

[InstSimplify] (a ^ b) ? (~a) ^ b : a ^ (~b) #63104

k-arrows opened this issue Jun 4, 2023 · 7 comments

Comments

@k-arrows
Copy link

k-arrows commented Jun 4, 2023

Test code:

int foo(int a, int b)
{
    return (a ^ b) ? (~a) ^ b : a ^ (~b) ;
}

Clang 15.0.0 (and Gcc trunk)

foo(int, int):                               # @foo(int, int)
        mov     eax, edi
        xor     eax, esi
        not     eax
        ret

Clang 16.0.0 (and Clang trunk)

foo(int, int):                               # @foo(int, int)
        mov     ecx, edi
        xor     ecx, esi
        cmp     edi, esi
        not     ecx
        mov     eax, -1
        cmovne  eax, ecx
        ret

https://godbolt.org/z/qdhKnj3af

@dc03
Copy link
Member

dc03 commented Jun 9, 2023

I'd like to fix this, but I'm not able to find the exact transform in InstCombine causing this. It appears that the newer clang is converting a ^ (~b) into 0 ^ (-1) (which then gets folded into -1), possibly because it is able to prove that a ^ b being false means that a and b are equal. The older clang appears to be converting a ^ (~b) as well as (~a) ^ b into ~(a ^ b), then coalescing them leading to the codegen you see.

@junaire
Copy link
Member

junaire commented Jun 9, 2023

This fold doesn't introduce new instructions so what you really should take a look is InstructionSimplify, maybe

static Value *simplifySelectWithICmpCond(Value *CondVal, Value *TrueVal,

Alive proof: https://alive2.llvm.org/ce/z/KEt9R5

@nikic
Copy link
Contributor

nikic commented Jun 9, 2023

This probably needs a generalization of simplifyWithOpReplaced to look at more than one instruction.

@dc03
Copy link
Member

dc03 commented Jun 21, 2023

This probably needs a generalization of simplifyWithOpReplaced to look at more than one instruction.

@nikic: Not entirely sure where this plays in. Stepping through the code, the transformation comes from here: a ^ ~b gets turned into a ^ (b ^ -1) which then gets reassociated by SimplifyAssociativeOrCommutative here to be (a ^ b) ^ -1, where a ^ b then gets transformed by simplifyByDomEq into 0. I don't see simplifyWithOpReplaced in the call stack at any point.

@nikic
Copy link
Contributor

nikic commented Jun 21, 2023

@dc03 The optimization regression comes from making use of dominating conditions in icmp simplification. However, we don't want to avoid that and instead make sure that the new IR can still be simplified. simplifyWithOpReplaced() is the transform that handles this kind of pattern.

@vfdff
Copy link
Contributor

vfdff commented Jun 21, 2023

The regression between llvm 15 and llvm 16 , https://gcc.godbolt.org/z/Goq9ETh67

@vfdff
Copy link
Contributor

vfdff commented Jun 24, 2023

maybe this is the simplify case https://alive2.llvm.org/ce/z/TGgJTq , show that the simplifyWithOpReplaced need look at more than one instruction.

@nikic nikic closed this as completed in 3d199d0 Jul 14, 2023
nikic added a commit that referenced this issue Jul 18, 2023
A similar assumption as for the x^x case also existed for the absorber
case, which lead to a stage2 miscompile. That assumption is not fixed.

-----

Support replacement of operands not only in the immediate
instruction, but also instructions it uses.

To the most part, this extension is straightforward, but there are
two bits worth highlighting:

First, we can now no longer assume that if the Op is a vector, the
instruction also returns a vector. If Op is a vector and the
instruction returns a scalar, we should consider it as a cross-lane
operation.

Second, for the x ^ x special case and the absorber special case, we
can no longer assume that one of the operands is RepOp, as we might
have a replacement higher up the instruction chain.

There is one optimization regression, but it is in a fuzzer-generated
test case.

Fixes #63104.
jdoerfert pushed a commit to jdoerfert/llvm-project that referenced this issue Jul 24, 2023
A similar assumption as for the x^x case also existed for the absorber
case, which lead to a stage2 miscompile. That assumption is not fixed.

-----

Support replacement of operands not only in the immediate
instruction, but also instructions it uses.

To the most part, this extension is straightforward, but there are
two bits worth highlighting:

First, we can now no longer assume that if the Op is a vector, the
instruction also returns a vector. If Op is a vector and the
instruction returns a scalar, we should consider it as a cross-lane
operation.

Second, for the x ^ x special case and the absorber special case, we
can no longer assume that one of the operands is RepOp, as we might
have a replacement higher up the instruction chain.

There is one optimization regression, but it is in a fuzzer-generated
test case.

Fixes llvm#63104.
veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Sep 6, 2024
Support replacement of operands not only in the immediate
instruction, but also instructions it uses.

To the most part, this extension is straightforward, but there are
two bits worth highlighting:

First, we can now no longer assume that if the Op is a vector, the
instruction also returns a vector. If Op is a vector and the
instruction returns a scalar, we should consider it as a cross-lane
operation.

Second, for the x ^ x special case, we can no longer assume that
the operand is RepOp, as we might have a replacement higher up the
instruction chain.

There is one optimization regression, but it is in a fuzzer-generated
test case.

Fixes llvm/llvm-project#63104.
veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Sep 6, 2024
A similar assumption as for the x^x case also existed for the absorber
case, which lead to a stage2 miscompile. That assumption is not fixed.

-----

Support replacement of operands not only in the immediate
instruction, but also instructions it uses.

To the most part, this extension is straightforward, but there are
two bits worth highlighting:

First, we can now no longer assume that if the Op is a vector, the
instruction also returns a vector. If Op is a vector and the
instruction returns a scalar, we should consider it as a cross-lane
operation.

Second, for the x ^ x special case and the absorber special case, we
can no longer assume that one of the operands is RepOp, as we might
have a replacement higher up the instruction chain.

There is one optimization regression, but it is in a fuzzer-generated
test case.

Fixes llvm/llvm-project#63104.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants