[PATCH] D152063: [AVR] Support left-rotations

Mon Jun 5 05:09:58 PDT 2023

aykevl added a comment.

Sorry I only see this now, after it has been merged.

> Curiously, `fshl.u16` (and larger types) work, similarly as all fshrs, which I think is related to the fact that `ROLBRd` is hard-coded to require `GPR8:$zero` in AVRInstrInfo.td.
>
> Now, I'm not really sure why it's been done this way and so I'm not 100% sure my approach to solving this problem is correct as well - but adjusting `ROLBRd` to work similarly as `RORBRd` seems to do the job.

There is a reason, namely that the instruction uses `R1` and now doesn't say so in InstrInfo. I have added the use of `R1` in D117426 <https://reviews.llvm.org/D117426>. With the use removed, interrupts may be miscompiled because `R1` might not be saved/cleared/restored inside an interrupt when a ROLB pseudo instruction is used inside an interrupt.

In other words, the zero register dependency is required for correctness and this patch will lead to miscompilation in some edge cases.

Here is an example that miscompiles on current trunk: https://godbolt.org/z/6ha5rGqY3 (`r1` is used but not saved/cleared).

================
Comment at: llvm/test/CodeGen/AVR/pseudo/ROLBrd.mir:24-28
-
-    ; avrtiny variant
-    ; CHECK:      $r14 = ADDRdRr killed $r14, killed $r14, implicit-def $sreg
-    ; CHECK-NEXT: $r14 = ADCRdRr $r14, $r17, implicit-def dead $sreg, implicit killed $sreg
-    $r14 = ROLBRd $r14, $r17, implicit-def $sreg
----------------
Why did you remove this test? I think it's useful (r1 vs r17).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152063