Paul,
You're right, and the answer to your question is yes: with the
decrement moved ahead of the jump, compare elimination picks up the
condition codes from the dec and no compare is emitted. v2 of 2/3
attached, doing it that way (addhi3 -1 first, then
emit_cmp_and_jump_insns LT 0). What 13.3.0 generates for
x >> n (unsigned int):
mov 02(sp),r0
mov 04(sp),r1
dec r1
bmi L_2
clc
ror r0
neg r1
ash r1,r0
L_2:
rts pc
and the SImode version does the same around ror/ror + ashc. It
even chose bmi over blt, which is fine here: a valid count is
0..15, so the decrement can't overflow and N alone decides.
One instruction shorter than v1 and the same on the fast path. The
testcase now scans for the two bmi's and for the absence of any
compare, so it will catch a regression to either the missing guard
or a redundant cmp.
The v2 is also attached to PR 126190, superseding the earlier
patch.
James
On Sat, Jul 11, 2026 at 4:26 PM Paul Koning <[email protected]> wrote:
>
> Hi James,
>
> Thanks!
>
> Patches 1 and 3 look good.
>
> I have a question about patch 2. It seems to do more work than is necessary.
> Instead of the compare against 0, you could do the decrement and branch if
> LT zero (bmi). Or does that actually end up what the code generates? It
> does a decent job of handling condition codes as a side effect of arithmetic
> operations, but perhaps not to the point of shuffling code around. In other
> words, if you move the gen_addhi3 earlier, and follow it by a cmp_and_jump
> against 0 with condition LT, that should end up not generating a compare but
> just using the conditions as set by the dec preceding (from the addhi3).
>
> paul
>
> > On Jul 10, 2026, at 4:31 PM, Migraine Man <[email protected]> wrote:
> >
> > Paul,
> >
> > Apologies -- the patches didn't make it out with the intro mail.
> > All three are attached here, and each is also attached to its
> > bugzilla PR (target/125058, target/126190, target/126191).
> >
> > Re the no-EIS variants: checked, and they are fine. On a non-EIS
> > target pdp11_expand_shift never returns false: small constant
> > counts (0..3) take the unrolled single-bit _sc patterns, and
> > everything else becomes the dec/bne single-bit loop, with a ble
> > guard for variable counts so a runtime count of 0 falls through
> > unchanged. None of the three broken branches -- all of which emit
> > ash/ashc -- is reachable without TARGET_40_PLUS, and none of the
> > fixes touches code reachable at -m10.
> >
> > I also confirmed by compiling the three reproducers with -m10:
> >
> > unsigned char >> n: clr/bisb zero-extend, ble guard, asr loop
> > (shifts right, count 0 handled)
> > unsigned int >> n: ble guard, clc/ror loop (count 0 handled)
> > signed char >> 4: movb (sign-extending), asrb x4
> > (sign preserved)
> >
> > One unrelated observation from reading that code, for completeness:
> > pdp11_assemble_shift emits the leading clc/ror(b) for LSHIFTRT
> > before decrementing n, so a *constant* count of 0 would shift by 1.
> > But expand_shift_1 (expmed.cc) returns the operand unchanged for a
> > literal zero count, so I don't believe that RTL can be formed; I
> > haven't tried to fix what I can't trigger.
> >
> > Fuzix is Alan Cox's UNIX-like OS for small machines (fuzix.org) --
> > a V7-flavored kernel and userland that runs on Z80, 6502, 68k, and
> > similar. I'm doing the PDP-11 port, running on an 11/20-compatible-plus-MMU
> > FPGA implementation I built, which is how these surfaced: PR 126190
> > was corrupting every odd single-indirect block of every file read.
> > Link to the pull-request discussion:
> > https://codeberg.org/EtchedPixels/FUZIX/pulls/1205
> >
> > Best regards,
> > James McGuire
> >
> >
> > On Fri, Jul 10, 2026 at 9:55 AM Paul Koning <[email protected]> wrote:
> >>
> >> Wow, thanks. I didn't see the actual patches, just this intro email.
> >>
> >> Did you check the no-EIS variants of these expanders to see if they are
> >> similarly broken?
> >>
> >> Btw, what is Fuzix? A pdp11 Unix?
> >>
> >> paul
> >>
> >>> On Jul 9, 2026, at 5:26 PM, Migraine Man <[email protected]> wrote:
> >>>
> >>> Paul,
> >>>
> >>> This series fixes three independent wrong-code bugs in the pdp11
> >>> shift expanders in config/pdp11/pdp11.md, found while bringing up
> >>> Fuzix on a PDP-11/20-compatible FPGA system built with gcc 13.3.0
> >>> as the cross-compiler. All three are still present in master.
> >>>
> >>> 1/3 - PR target/125058: the QImode branch of lshr<mode>3 does not
> >>> negate the shift count, so a logical right shift is emitted as a
> >>> LEFT shift.
> >>>
> >>> 2/3 - PR target/126190: the HImode/SImode variable-count branch of
> >>> lshr<mode>3 expands as a one-bit logical shift followed by an
> >>> arithmetic shift of (count - 1), which is invalid for a runtime
> >>> count of 0: it computes x & ~1. In the field this corrupted every
> >>> odd single-indirect block of every Fuzix file read (bn >> sh with
> >>> sh = 0).
> >>>
> >>> 3/3 - PR target/126191: the QImode branch of ashr<mode>3
> >>> zero-extends the operand before the arithmetic shift, losing the
> >>> sign for counts 4..7 (smaller counts take the asrb path). The fix
> >>> (sign-extend) also makes the emitted code two words shorter.
> >>>
> >>> Each patch adds a scan-assembler test under gcc.target/pdp11.
> >>> The combined fixes have been running in a self-hosted gcc 13.3.0
> >>> cross toolchain building the Fuzix kernel, libc, and userland;
> >>> codegen verified case by case on the PRs.
> >>>
> >>> Best regards,
> >>> James McGuire
> >>
> > <0003-pdp11-sign-extend-the-operand-in-QImode-arithmetic-r.patch><0002-pdp11-guard-variable-logical-right-shift-against-a-z.patch><0001-pdp11-negate-the-count-in-QImode-logical-right-shift.patch>
>
From f1fc8ab33c23e32a91a21cb2fc0381ccfaf7338c Mon Sep 17 00:00:00 2001
From: James McGuire <[email protected]>
Date: Thu, 9 Jul 2026 16:41:29 -0400
Subject: [PATCH v2] pdp11: guard variable logical right shift against a zero
count [PR126190]
The non-constant HImode/SImode branch of lshr<mode>3 expands
x >> n as a one-bit logical shift followed by an arithmetic shift
of (n - 1). That identity only holds for n >= 1: for a runtime
count of zero it computes x & ~1. In the field this corrupted
every odd-numbered single-indirect block of every file read on a
Fuzix PDP-11 port (bn >> sh with sh = 0 in bmap()).
Decrement the count first and branch around the shift pair if it
went negative; compare elimination reuses the condition codes the
decrement sets, so the guard costs a single branch and no compare
instruction (suggested by Paul Koning).
gcc/ChangeLog:
PR target/126190
* config/pdp11/pdp11.md (lshr<mode>3): Guard the variable
count expansion against a runtime count of zero.
gcc/testsuite/ChangeLog:
PR target/126190
* gcc.target/pdp11/lshr-zero.c: New test.
Signed-off-by: James McGuire <[email protected]>
---
gcc/config/pdp11/pdp11.md | 28 +++++++++++++++++-----
gcc/testsuite/gcc.target/pdp11/lshr-zero.c | 22 +++++++++++++++++
2 files changed, 44 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/pdp11/lshr-zero.c
diff --git a/gcc/config/pdp11/pdp11.md b/gcc/config/pdp11/pdp11.md
index a2cbd35..d19c6bd 100644
--- a/gcc/config/pdp11/pdp11.md
+++ b/gcc/config/pdp11/pdp11.md
@@ -1784,17 +1784,33 @@
}
else
{
- r = gen_reg_rtx (<QHSint:mname>);
- emit_insn (gen_lshiftrt<mode>_sc (r, operands[1], const1_rtx));
if (GET_CODE (operands[2]) != CONST_INT)
{
- n = gen_reg_rtx (HImode);
- emit_insn (gen_addhi3 (n, operands [2], GEN_INT (-1)));
- emit_insn (gen_ashr<mode>3 (operands[0], r, n));
+ /* The one-bit logical shift followed by an arithmetic
+ shift of (count - 1) is only valid for count >= 1;
+ a runtime count of 0 must leave the value unchanged.
+ Decrement first and skip the shift pair if the
+ result went negative - compare elimination then
+ reuses the condition codes the decrement set, so no
+ separate compare is emitted. */
+ rtx skip = gen_label_rtx ();
+ r = copy_to_mode_reg (<QHSint:mname>, operands[1]);
+ n = copy_to_mode_reg (HImode, operands[2]);
+ emit_insn (gen_addhi3 (n, n, GEN_INT (-1)));
+ emit_cmp_and_jump_insns (n, const0_rtx, LT, NULL_RTX,
+ HImode, 0, skip);
+ emit_insn (gen_lshiftrt<mode>_sc (r, r, const1_rtx));
+ emit_insn (gen_ashr<mode>3 (r, r, n));
+ emit_label (skip);
+ emit_move_insn (operands[0], r);
}
else
- emit_insn (gen_asl<QHSint:hmode>_op (operands[0], r,
+ {
+ r = gen_reg_rtx (<QHSint:mname>);
+ emit_insn (gen_lshiftrt<mode>_sc (r, operands[1], const1_rtx));
+ emit_insn (gen_asl<QHSint:hmode>_op (operands[0], r,
GEN_INT (1 - INTVAL (operands[2]))));
+ }
}
}
DONE;
diff --git a/gcc/testsuite/gcc.target/pdp11/lshr-zero.c b/gcc/testsuite/gcc.target/pdp11/lshr-zero.c
new file mode 100644
index 0000000..668744a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/pdp11/lshr-zero.c
@@ -0,0 +1,22 @@
+/* PR target/126190: HImode/SImode logical right shift by a variable
+ count computed (x >> 1) << 1 = x & ~1 for a runtime count of 0. */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+unsigned int
+hshift (unsigned int x, int n)
+{
+ return x >> n; /* hshift (19, 0) must return 19, not 18. */
+}
+
+unsigned long
+sshift (unsigned long x, int n)
+{
+ return x >> n;
+}
+
+/* Both functions guard the shift pair by decrementing the count
+ and branching on the decrement's own condition codes - no
+ separate compare instruction should be emitted. */
+/* { dg-final { scan-assembler-times "bmi" 2 } } */
+/* { dg-final { scan-assembler-not "cmp" } } */
--
2.43.0
