IDE 1.8.6 / 1.8.7 Compilation Issues

actually it’s assembly with somemixed C.

A couple of weeks ago I looked into the drawPixel (and optimized the C bit a bit) as I noticed the compiler didn’t optimize out an unused pointer (didn’t solve that though)

@filmote if you zip up your project I’ll have a go and compare at it.

In case anyones wondering what’s optimized. I got rid of the else case and used a different masking technique (It will be featured in the next homemade package update)

if (color) {
    sBuffer[row_offset] |=   bit;
  } else {
    sBuffer[row_offset] &= ~ bit;
}

To:

  uint8_t data = sBuffer[row_offset] |= bit;
  if (!color) data ^= bit;
  sBuffer[row_offset] = data;

When you’ve got time again and picking up your Arduboy2 library, give me a ping and I’ll sort out all the optimisations I’ve added to simplify things.

2 Likes

Does that need to be sBuffer[row_offset] |= bit?
You’re writing it back anyway so it could just be sBuffer[row_offset] | bit, couldn’t it?

uint8_t data = sBuffer[row_offset] | bit;
if (!color) data ^= bit;
sBuffer[row_offset] = data;

(The compiler will probably optimise the write out at the SSA stage anyway, but better safe than sorry.)

1 Like

Ha! right missed that. Thanks for pointing that out.

1 Like

Its the Blackjack project from here > https://github.com/Press-Play-On-Tape/Blackjack

1 Like

Thanks! and… I found the glitch.

The compiler somehow decides to store bitshift_left array in ram rather then in progmem. But since the drawPixel assembly code reads from progmem, ‘random’ bitmask data is used instead.

I think the problem arises here

I don’t know if it’s an actual compiler glitch or incorrect use of C(++) @Pharap any ideas?

1 Like

Nice pickup … I am glad I could isolate for you!

2 Likes

Well, I can start with some information at least.
PROGMEM is actually implemented as __attribute__((__progmem__)).

It’s plausible that a newer GCC version has affected where the attribute can be placed, but I would hope not because that contradicts the docs.

It certainly sounds like a bug of some description, possibly in GCC rather than the Arduino IDE.

1 Like

I think I may have a nice alternative that may save a few bytes too at the cost of 1 more cycle. Will test it first though

1 Like

Now I think about it, I’m not sure the bsl variable is actually needed anyway.
Registers are values, not references, so copying the value of bitshift_left straight to z should have had the same effect without needing the intermediary variable.

The compiler will start to complain about it being read only and ‘+z’ is used.

I found a little more about the glitch. It looks like the compiler optimized out bitshift_left and uses BIT_SHIFT instead which used in Ardbitmap and stored in ram.

Edit:

Okay my idea works :smiley: the bit conversion code takes the same number of cycles, 1 instruction more but doesn’t need the byte array, saving 6 bytes.

   //bit = 1 << (y & 7)
    "ldi  %[bit], 1                    \n" bit = 1;
    "sbrc %[y], 1                      \n" if (y & _BV(1)) bit = 4;
    "ldi  %[bit], 4                    \n"
    "sbrc %[y], 0                      \n" if (y & _BV(0)) bit = bit << 1;
    "lsl  %[bit]                       \n"
    "sbrc %[y], 2                      \n" if (y & _BV(2)) bit = (bit << 4) | (bit >> 4);
    "swap %[bit]                       \n" 
2 Likes

I am not sure what you mean here but are you saying Ardbutmap is the issue? Or that using Ardbitmao contributes to the issue?

bitshift_left (arduboy2) and BIT_SHIFT (Ardbitmap) both contain the same data. only the 1st is in progmem and the 2nd in ram.

When reversing the code there is only bitshift_left in ram and no BIT_SHIFT at all.

When removing bitshift_left (because of using my alternative code) then after reversing the code there is only BIT_SHIFT in ram

Ok here’s the optimized drawPixel that works fine with 1.8.7 :

void Arduboy2Base::drawPixel(int16_t x, int16_t y, uint8_t color)
{
  #ifdef PIXEL_SAFE_MODE
  if (x < 0 || x > (WIDTH-1) || y < 0 || y > (HEIGHT-1))
  {
    return;
  }
  #endif

  uint16_t row_offset;
  uint8_t bit;

  asm volatile
  (
    // bit = 1 << (y & 7)
    "ldi  %[bit], 1                    \n" //bit = 1;
    "sbrc %[y], 1                      \n" //if (y & _BV(1)) bit = 4;
    "ldi  %[bit], 4                    \n"
    "sbrc %[y], 0                      \n" //if (y & _BV(0)) bit = bit << 1;
    "lsl  %[bit]                       \n"
    "sbrc %[y], 2                      \n" //if (y & _BV(2)) bit = (bit << 4) | (bit >> 4);
    "swap %[bit]                       \n" 
    //row_offset = y / 8 * WIDTH + x;
    "andi %A[y], 0xf8                  \n" // 
    "mul  %[width_offset], %A[y]       \n"
    "movw %[row_offset], r0            \n"
    "clr  __zero_reg__                 \n"
    "add  %A[row_offset], %[x]         \n"
#if WIDTH != 128
    "adc  %B[row_offset], __zero_reg__ \n" // only none 128 width can overflow
#endif
    : [row_offset]   "=&x" (row_offset),   // upper register (ANDI)
      [bit]          "=&d" (bit),          // upper register (LDI)
      [y]            "+d"  (y)             // upper register (ANDI), must be writable
    : [width_offset] "r"   ((uint8_t)(WIDTH/8)),
      [x]            "r"   ((uint8_t)x)
    :
  );
  uint8_t data = sBuffer[row_offset] | bit;
  if (!color) data ^= bit;
  sBuffer[row_offset] = data;
}
4 Likes

Can you create a GitHub PR for this? I’ll then apply it and make a new release.

If you don’t want to do a PR you can raise an issue with the code included. Or, I can just pull it from here. I can then apply your code and credit you.

I’ll create a PR

Edit:

created

1 Like

That’s annoying. There ought to be a way to signal ‘copy the value, but don’t write back to the variable’.

@filmote, @Mr.Blinky, @Pharap,

I’ve incorporated @Mr.Blinky’s PR that contains the fixes (and optimisations) to drawPixel() for this problem. PRs by @Pharap, adding a new generateRandomSeed() function and enhancements for collision detection features, are also included.

The new version is labelled 5.2.0 and is on the master branch in the GitHub repository.

I’ve done some quick testing but would appreciate it if any of you, or anyone else, would test it with code that exhibited the problem and any other code that you see fit.

If everything appears to be OK with the changes, I’ll tag and release it.

Thanks.

2 Likes

Nice work … I will give it a go soon.

What are these?

Just some minor changes that @Pharap contributed:

  • Added default and fully initialising constructors for the Point and Rect objects.
  • Made the collide() functions static.

Basically the following is now valid:

// 1) Point(x, y) ctor
Point point = Point(1, 2);

// 2) Point() ctor
Point pointB = Point();

// 3) Rect(x, y, w, h) ctor
Rect rect = Rect(2, 3, 4, 5);

// 4) Rect() ctor
Rect rectB = Rect();

// 5) static collide(Point, Rect) overload
if(Arduboy2::collide(point, rect))
{
	// ect
}

// 6) static collide(Rect, Rect) overload
if(Arduboy2::collide(rectB, rect))
{
	// ect
}
1 Like