[PATCH] D96957: [AVR] Fix rotate instructions

[Ayke via Phabricator via llvm-commits]

aykevl added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:1349
+    .addReg(DstReg)
+    .addImm(7);
 
----------------
benshi001 wrote:
> BLD has a implict SREG operand, do we need to set it as killed?
No, the kill flag is optional. See https://youtu.be/objxlZg01D0?t=2379 for details.

Also, I believe setting the kill flag on the SREG does not make a lot of sense, as the SREG is not an allocatable register and in any case it's not true that after that no bits of the SREG register are relevant anymore (it contains the interrupt enable flag, which is always relevant).


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:1753
 
+  // 8-bit ROL is an alias of ADC Rd, Rd
+
----------------
benshi001 wrote:
> remove this comment line? It is incorrect.
I have removed it. I think it is meant to say that the AVR rol instruction is an alias and thus the ROLB/RORB instructions are needed, but it is only confusing now.


================
Comment at: llvm/test/CodeGen/AVR/rot.ll:40
 ; CHECK-NEXT: .LBB1_1:
-  ; CHECK-NEXT: lsr r24
-  ; CHECK-NEXT: ldi r0, 0
-  ; CHECK-NEXT: ror r0
-  ; CHECK-NEXT: or r24, r0
+  ; CHECK-NEXT: bst r24, 0
+  ; CHECK-NEXT: ror r24
----------------
benshi001 wrote:
> use llvm/utils/update_llc_test_checks.py to update instruction pattern?
I could do that, but it would introduce a lot of noise to this patch. That said, I think it would be a good idea to convert most codegen tests to be autogenerated using update_llc_test_checks.py, but perhaps in a separate (non-functional) patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96957