Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Missed optimization to use the carry flag after subtracting #45778

Open Quuxplusone opened 4 years ago

Quuxplusone commented 4 years ago
Bugzilla Link PR46809
Status NEW
Importance P normal
Reported by Joseph C. Sible (josephcsible@gmail.com)
Reported on 2020-07-22 10:19:40 -0700
Last modified on 2020-11-12 07:05:31 -0800
Version 10.0
Hardware PC Linux
CC craig.topper@gmail.com, laytonkifer@gmail.com, llvm-bugs@lists.llvm.org, llvm-dev@redking.me.uk, spatel+llvm@rotateright.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
Consider these two C functions:

unsigned f1(unsigned x, unsigned y) {
    unsigned z = x - y;
    if (x < y) {
        z += 100;
    }
    return z;
}
unsigned f2(unsigned x, unsigned y) {
    unsigned z;
    if (__builtin_usub_overflow(x, y, &z)) {
        z += 100;
    }
    return z;
}

At -O3 or -Os, they both generate the same assembly:

        movl    %edi, %eax
        movl    %edi, %ecx
        subl    %esi, %ecx
        addl    $100, %ecx
        subl    %esi, %eax
        cmovbl  %ecx, %eax
        retq

https://godbolt.org/z/MevEM9

That's suboptimal. They should have instead used lea to save a move, save a
duplicate subtraction, and avoid clobbering the carry flag, like this:

        subl    %esi, %edi
        leal    100(%rdi), %eax
        cmovael %edi, %eax
        retq
Quuxplusone commented 4 years ago
Loosening this heuristic as the TODO suggests gets this case

diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index 3cd80cb..88f0a10 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -2601,7 +2601,7 @@ bool X86DAGToDAGISel::selectLEAAddr(SDValue N,
     // TODO: This could be an 'or' rather than 'and' to make the transform more
     //       likely to happen. We might want to factor in whether there's a
     //       load folding opportunity for the math op that disappears with LEA.
-    if (isMathWithFlags(N.getOperand(0)) && isMathWithFlags(N.getOperand(1)))
+    if (isMathWithFlags(N.getOperand(0)) || isMathWithFlags(N.getOperand(1)))
       Complexity++;
   }
Quuxplusone commented 4 years ago

Is there a better place to check for this that wouldn't be a heuristic?