[PATCH] D78439: [AVR] Fix miscompilation of zext + add

[Ayke via Phabricator via llvm-commits]

aykevl added a comment.

It took me much longer than it should to find a solid reproducer, but here is one:

  // in one file
  uint32_t add(uint32_t a, uint16_t b) {
      return a + b;
  }
  
  // in another file
  uint32_t add(uint32_t a, uint16_t b);
  
  
  // somewhere in the code of that file
  uart_printint("add", add(0x7ffff, 1));

Adding 0x7ffff and 1 should result in 0x80000. However, without this patch it results in 0x60000.

Assembly without this patch:

  d20:       64 0f           add     r22, r20
  d22:       75 1f           adc     r23, r21
  d24:       80 40           sbci    r24, 0x00       ; 0
  d26:       90 40           sbci    r25, 0x00       ; 0
  d28:       08 95           ret

With this patch:

  d1c:       20 e0           ldi     r18, 0x00       ; 0
  d1e:       30 e0           ldi     r19, 0x00       ; 0
  d20:       64 0f           add     r22, r20
  d22:       75 1f           adc     r23, r21
  d24:       82 1f           adc     r24, r18
  d26:       93 1f           adc     r25, r19
  d28:       08 95           ret



> A subtraction of zero is always redundant, therefore the two SBCIs from the first patch are completely redundant, and could be removed by the compiler in a future optimization.

The `sbci` not only subtracts an immediate (in this case zero), but it also subtracts the carry bit. Therefore it definitely does have an effect. This also is true for the last two `adc` instructions: they are needed to add the carry bit in case the previous add instruction caused a wraparound. However `sbci` and `adc` use the carry in the opposite direction (subtracting or adding it to their result).

Looking at AVRInstrInfo.td, I suspect there is a much larger number of these bugs around `sbci`.

I found this bug while trying to add AVR support to compiler-rt. One instance of the bug is here <https://github.com/llvm/llvm-project/blob/release/10.x/compiler-rt/lib/builtins/muldi3.c#L24> (the `+=`). I originally found it somewhere else, but had trouble finding the exact location again.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78439/new/

https://reviews.llvm.org/D78439