Use of bitfield structures with "reserved" components needlessly defeats compiler optimizations. (IDFGH-13822)

[WestfW]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

The SOC include files xxx_struct.h are careful to define "reserved" fields for unused bits in registers, but this can force the compiler to do unnecessary read/modify/write sequences on the whole register when a simple store is sufficient and more correct. It may also interfere with atomicity when accessing registers that are specifically design to provide atomic access.

I first noticed this in gpio_ll_set_level() for ESP32C3, which reads (

esp-idf/components/hal/esp32c3/include/hal/gpio_ll.h

Line 339 in 46acfdc

static inline void gpio_ll_set_level(gpio_dev_t *hw, uint32_t gpio_num, uint32_t level)

):

__attribute__((always_inline))
static inline void gpio_ll_set_level(gpio_dev_t *hw, uint32_t gpio_num, uint32_t level)
{
    if (level) {
        hw->out_w1ts.out_w1ts = (1 << gpio_num);
    } else {
        hw->out_w1tc.out_w1tc = (1 << gpio_num);
    }
}

This SHOULD compile to a simple store of a constant to the GPIO_OUT_W1TS/GPIO_OUT_W1TC registers, but instead compiles to (slow, long, and not atomic)

        hw->out_w1tc.out_w1tc = (1 << gpio_num);
4200005e:	fc0006b7          	lui	a3,0xfc000   ;; mask of reserved bits
42000062:	47d8                	lw	a4,12(a5)
42000064:	8f75                	and	a4,a4,a3
42000066:	00476713          	ori	a4,a4,4    ;; actual bit being written
4200006a:	c7d8                	sw	a4,12(a5)

This is because the low-level structure definition ( https://github.com/espressif/esp-idf/blob/master/components/soc/esp32c3/include/soc/gpio_struct.h#L37 ) has:

    union {
        struct {
            uint32_t out_w1tc:  26;
            uint32_t reserved26: 6;
        };
        uint32_t val;
    } out_w1tc;

forcing the compiler to read the 32bit quantity at hw->out_w1tc so that it can modify JUST the unreserved bitfield.
(also, this happens to be a write-only register. Hopefully, it reads as 0.)

This is defeating the whole purpose of the out_w1tc and similar registers.

For this particular function (gpio_ll_set_level), a fix is to change the code to use the full 32bit version of the register:

   hw->out_w1tc.val = (1 << gpio_num);

Which then compiles to the expected:

        hw->out_w1tc.val = (1 << gpio_num);
   8:   4711                    li      a4,4
   a:   c7d8                    sw      a4,12(a5)

However, I notice that there are 1000+ uses of "reserved" in the various */xxx_struct.h files that may be causing similar non-optimal code, so it might be worth looking into as a more general issue.

[jrahlf]

This is also the case for esp32s3 gpio_ll functions.
Hello Espressif, can we get someone assigned for this? @igrr ?

[igrr]

Hi, this issue is already in the backlog of one of the teams. I think it didn't get picked up quickly because the scope mentioned in the title (coming up with a different approach to reserved fields) is pretty broad, while the effect of the issue in the case of GPIO registers is only reduced performance. Besides, LL layer is considered to be a private API, not something that applications will generally use. Based on these factors, the priority of this issue was seen as low.

Let me see if we can first pick up the simpler task of fixing gpio_ll_set_level to use out_w1t*.val instead of the bitfield.