[z80] Dead code generated

[Job Bolle]

Good evening,
I am trying to implement rtrackUpdate() for Z80.
(I will probably have questions about this later...)

While testing, I came across a case of dead code being generated.
I made it so that a conditional jump not taken adds new knowledge about the CPU state. In this case, the new knowledge (bit 7 of D is set) conflicts with existing knowledge (D is zero), which I assumed indicated an error on my part, but it turns out that it indicated dead code.

The following C code:

unsigned char *buf;

void dead(unsigned char arg1, unsigned char arg2, unsigned char arg3)
{
    unsigned short idx;

    idx = 60 * arg2 + arg1 / 4;
    buf[idx] = arg3;
}

Generates the following Z80 code:

    ...
    add hl, hl
    add hl, hl
    ex  (sp), hl
    ld  d, #0x00    ; load D with zero
    ld  c, e        ; does not affect D
    ld  b, d        ; does not affect D
    bit 7, d        ; test bit 7 of D, which we know is not set
    jp  Z, 00103$   ; therefor this jump is always taken
    ld  c, e        ; so
    ld  b, d        ;  this
    inc bc          ;   code
    inc bc          ;    is
    inc bc          ;     dead
00103$:
    sra b           ; here it starts a 16-bit right shift by 2
    rr  c
    sra b
    ...

I guess that it wants to treat arg1 as a 16-bit signed int in performing arg1 / 4, since it has already decided that 4 is a 16-bit signed int, and the bit 7 test is to determine whether it is negative.
This is a bit unfortunate, since at compile time it is known that 4 is not negative, and fits in 8 bits, and the other operand of the division is unsigned 8 bit.

If I change 4 to 4u, making it explicitly unsigned, the dead part goes away:

idx = 60 * arg2 + arg1 / 4u;

    ...
    add hl, hl
    add hl, hl
    ld  b, #0x00
    srl b
    rr  c
    srl b
    rr  c
    add hl, bc
    ...

If I cast 4 to an unsigned char, the 16-bit shift becomes an 8-bit shift:

idx = 60 * arg2 + arg1 / (unsigned char)4;

    ...
    add hl, hl
    add hl, hl
    srl c
    srl c
    ld  b, #0x00
    add hl, bc
    ...

... this looks much nicer.

Regards,
-Job

Edit to add: I am on 13724, which is HEAD at time of writing this.

[Philipp Klaus Krause]

When wondering why asm is geneated (in particular, which istruction comes from which part of the input), it is often helpful to try --no-peep, --fverbose-asm, --i-code-in-asm or a combination thereof.

[Job Bolle]

geniCodeDivision() can do a simple right shift if left is unsigned and right is a power of two, but usualBinaryConversions() makes left signed if right is signed, as it casts both operands to the result type.

The test for unsignedness of left could be done on the original left before it gets cast by usualBinaryConversions.

[Job Bolle]

The test for unsignedness of left could be done on the original left

This gets rid of the negativity test and the conditional addition:

--- a/src/SDCCicode.c
+++ b/src/SDCCicode.c
@@ -2256,6 +2256,7 @@ geniCodeDivision (operand *left, operand *right, RESULT_TYPE resultType, bool pt
 {
   iCode *ic;
   int p2 = 0;
+  bool left_unsigned = IS_UNSIGNED (getSpec (operandType (left)));
   sym_link *resType = usualBinaryConversions (&left, &right, resultType, '/');
   sym_link *rtype = operandType (right);
   sym_link *retype = getSpec (rtype);
@@ -2268,7 +2269,7 @@ geniCodeDivision (operand *left, operand *right, RESULT_TYPE resultType, bool pt
    it a right shift, too. */

   if (IS_LITERAL (retype) &&
-      (!IS_FLOAT (letype) && !IS_FIXED (letype) && IS_UNSIGNED (letype) || ptrdiffdiv) &&
+      (!IS_FLOAT (letype) && !IS_FIXED (letype) && left_unsigned || ptrdiffdiv) &&
       ((p2 = powof2 ((TYPE_TARGET_ULONG) ulFromVal (OP_VALUE (right)))) > 0))
     {
       ic = newiCode (RIGHT_OP, left, operandFromLit (p2));      /* right shift */
@@ -2278,7 +2279,7 @@ geniCodeDivision (operand *left, operand *right, RESULT_TYPE resultType, bool pt
      followed by right shift */
   else if (IS_LITERAL (retype) &&
       !IS_FLOAT (letype) &&
-      !IS_FIXED (letype) && !IS_UNSIGNED (letype) &&
+      !IS_FIXED (letype) && !left_unsigned &&
       ((p2 = powof2 ((TYPE_TARGET_ULONG) ulFromVal (OP_VALUE (right)))) > 0) &&
       (TARGET_Z80_LIKE || TARGET_HC08_LIKE || TARGET_MOS6502_LIKE))
     {

I'm still looking into the best and most proper way to use 8-bit instead of 16-bit arithmetic when possible.