[Bug c/68000] New: Suboptimal ternary operator codegen

[thaines.astro at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68000

Bug ID: 68000
   Summary: Suboptimal ternary operator codegen
   Product: gcc
   Version: 5.1.0
Status: UNCONFIRMED
  Severity: enhancement
  Priority: P3
 Component: c
  Assignee: unassigned at gcc dot gnu.org
  Reporter: thaines.astro at gmail dot com
  Target Milestone: ---

Created attachment 36534
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36534&action=edit
Preprocessed C code for "Suboptimal ternary operator codegen"

I think this problem is likely related to bug 23286, bug 56096, bug 64524, and
possibly bug 67879.

struct pair8 { uint8_t x,y; };
uint8_t foo_manual_hoist(struct pair8 *p) {
const uint8_t a = p->x + 1;
return a == p->y ? 0 : a;
}
uint8_t foo(struct pair8 *p) {
return p->x + 1 == p->y ? 0 : p->x + 1;
}
uint8_t foo_if(struct pair8 *p) {
uint8_t a = p->x + 1;
if (a == p->y)
a = 0;
return a;
}
int main() {}

Under gcc-5.1, gcc -m64 -march=native -O3 -g -S -masm=intel -o test.asm test.c
produces

foo_manual_hoist:
movzx eax, BYTE PTR [rdi+1]
mov   edx, 0
add   eax, 1
cmp   al, BYTE PTR [rdi+2]
cmove eax, edx
ret
foo:
movzx edx, BYTE PTR [rdi+1]
movzx ecx, BYTE PTR [rdi+2]
mov   eax, edx   // weird!
add   edx, 1
add   eax, 1
cmp   edx, ecx
mov   edx, 0
cmove eax, edx
ret
foo_if:
movzx eax, BYTE PTR [rdi+1]
mov   edx, 0
add   eax, 1
cmp   al, BYTE PTR [rdi+2]
cmove eax, edx
ret

As expect, the ternary version with the manual hoist of the common
subexpression produces identical codegen as the if version with the same hoist.
What is unexpected, is the duplication of the common subexpression for the
"bare" ternary version (line 3 of the disassembly of foo). It is clear that the
compiler sees the expressions to be the same, but treats them separately
anyway. As an aside, clang-3.6 produces identical code for all three versions.

[Bug middle-end/68000] Suboptimal ternary operator codegen

[thaines.astro at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68000

--- Comment #3 from Tim Haines  ---
(In reply to Andrew Pinski from comment #1)
> Note I think the trunk already has improved code generation.

Here is the codegen from the latest trunk build using the same options as
before.

foo_manual_hoist:
movzx eax, BYTE PTR [rdi]
mov   edx, 0
inc   eax
cmp   al, BYTE PTR [rdi+1]
cmove eax, edx
ret

foo:
movzx edx, BYTE PTR [rdi]
movzx ecx, BYTE PTR [rdi+1]
mov   eax, edx
inc   edx
cmp   edx, ecx
je.L6
inc   eax
ret
.L6:
xor   eax, eax
ret

foo_if:
movzx eax, BYTE PTR [rdi]
mov   edx, 0
inc   eax
cmp   al, BYTE PTR [rdi+1]
cmove eax, edx
ret

Changing from a cmove to a cmp/jmp doesn't change the instruction latency
(although I couldn't find the latency for je on intel. I assume it's 1 like on
AMD), but now the branch predictor will be invoked- bringing possible pipeline
hazards. I don't mean to be overly critical, but I wouldn't consider this to be
an improvement to the previous code- especially since the other two versions of
the function use cmove. As Marc noted, we are still missing CSE here.

NB: The structure offsets are different here because the assembly I originally
posted was poorly anonymized by me. Mea culpa!

[Bug c/7652] -Wswitch-break : Warn if a switch case falls through

[thaines.astro at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7652

Tim Haines  changed:

   What|Removed |Added

 CC||thaines.astro at gmail dot com

--- Comment #46 from Tim Haines  ---
(In reply to Marek Polacek from comment #45)
> Patches posted <https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00532.html>.

Thank you for putting in the work to make this happen! Does the current patch
allow calling a [[noreturn]] function without the need to follow it with a call
to __builtin_fallthrough()? Something like

int i = ...;
switch (i) {
case 1:
std::abort();
// don't warn about fallthrough here
case 2:
std::cout << "Hello world!\n";
break;
}