https://gcc.gnu.org/bugzilla/show_bug.cgi?id=128524
Bug ID: 128524
Summary: [m68k] dbmi/dbpl loop exits through the early-exit
branch when the result is zero (jle instead of
jmi/jpl)
Product: gcc
Version: 16.2.1
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: target
Assignee: unassigned at gcc dot gnu.org
Reporter: bernie at codewiz dot org
Target Milestone: ---
Created attachment 66103
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=66103&action=edit
testcase
output_dbcc_and_branch() in gcc/config/m68k/m68k.cc emits the wrong exit
branch for the two sign-only condition codes:
case PLUS:
output_asm_insn ("dbpl %0,%l1\n\tjle %l2", operands);
...
case MINUS:
output_asm_insn ("dbmi %0,%l1\n\tjle %l2", operands);
These come from the define_peepholes in m68k.md that turn
jCC label ; early exit
dbra dN,loop
into
dbCC dN,loop
jCC label
dbCC falls through either because CC holds or because the counter ran out,
and the jCC after it tells the two apart by re-testing the flags left by the
loop body. For mi that test must be jmi, and for pl it must be jpl. jle is
also taken when the loop runs out with a zero result (Z set), so such a
loop leaves through the early exit by mistake. Every other case in the
function already repeats its own condition (dbeq/jeq, dbne/jne, ...).
This miscompiles the over-subscription check in zlib's inflate_table()
(inftrees.c), so inflate rejects valid streams with "invalid code lengths
set":
left = 1;
for (len = 1; len <= MAXBITS; len++) {
left <<= 1;
left -= count[len];
if (left < 0) return -1;
}
At -O2 the loop becomes:
L3: add.l d0,d0
clr.l d2
move.w (a0)+,d2
sub.l d2,d0
dbmi d1,L3
jle L4 ; return -1, also taken when left == 0
clr.w d1
subq.l #1,d1
jpl L3
A complete Huffman code ends the loop with left == 0, so the function
returns -1.
The peephole only matches when the jCC and the dbra are adjacent. Here that
takes the count[] pointer's increment folding into a post-increment load.
With default options tree-sink keeps the increment apart, so the testcase
uses -fno-tree-sink to get the adjacent shape.
Proposed patch:
```diff
diff --git a/gcc/config/m68k/m68k.cc b/gcc/config/m68k/m68k.cc
--- a/gcc/config/m68k/m68k.cc
+++ b/gcc/config/m68k/m68k.cc
@@ -1787,11 +1787,11 @@ output_dbcc_and_branch (rtx *operands, rtx_code code)
break;
case PLUS:
- output_asm_insn ("dbpl %0,%l1\n\tjle %l2", operands);
+ output_asm_insn ("dbpl %0,%l1\n\tjpl %l2", operands);
break;
case MINUS:
- output_asm_insn ("dbmi %0,%l1\n\tjle %l2", operands);
+ output_asm_insn ("dbmi %0,%l1\n\tjmi %l2", operands);
break;
default:
```
Testcase:
gcc.target/m68k/prNNNNNN.c:
```c
/* { dg-do run } */
/* { dg-options "-O2 -fno-tree-sink" } */
/* zlib's inflate_table() checks a Huffman code for over-subscription with
this loop. The early exit tests the sign of LEFT, and the final peephole
that folds it into a dbmi must then leave the loop on mi, not le: a
complete code ends with LEFT at 0, which is not over-subscribed. */
extern void abort (void);
#define MAXBITS 15
__attribute__ ((noipa)) int
check_lengths (const unsigned short *count)
{
int left = 1;
unsigned len;
for (len = 1; len <= MAXBITS; len++)
{
left <<= 1;
left -= count[len];
if (left < 0)
return -1;
}
return left;
}
int
main (void)
{
unsigned short count[MAXBITS + 1] = { 0 };
/* Two codes of length 1: complete, LEFT ends at 0. */
count[1] = 2;
if (check_lengths (count) != 0)
abort ();
/* One code of length 1: incomplete, LEFT ends positive. */
count[1] = 1;
if (check_lengths (count) != 1 << (MAXBITS - 1))
abort ();
/* Three codes of length 1: over-subscribed. */
count[1] = 3;
if (check_lengths (count) != -1)
abort ();
return 0;
}
```
ChangeLog, for gcc-patches:
```
PR target/NNNNNN
* config/m68k/m68k.cc (output_dbcc_and_branch): Leave the PLUS and
MINUS loops with jpl and jmi, not jle.
gcc/testsuite/
PR target/NNNNNN
* gcc.target/m68k/prNNNNNN.c: New test.
```
Testing so far:
- Reproduced with an m68k-amigaos GCC 16.2 built from master plus target
patches (AmigaPorts 16.2-rc23): the testcase aborts at -O2 -fno-tree-sink
with -m68000, -m68020 and -m68040, and the generated code has dbmi/jle as
above. It passes once that jle is changed to jmi in the generated
assembly.
- Not yet run: an upstream m68k-linux-gnu compiler built from master with
the patch, and the m68k testsuite.
- The PLUS case is fixed by symmetry. I did not get the peephole to fire for
it with a quick variant of the loop, so the testcase covers MINUS only.
