[PATCH] D60365: [AVR] Fix codegen for rotate instructions

[Daan Sprenkels via Phabricator via llvm-commits]

dsprenkels marked 3 inline comments as done.
dsprenkels added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:1682
+                        "rolb\t$rd",
+                        [(set i8:$rd, (AVRrol i8:$src)), (implicit SREG)]>;
+
----------------
dylanmckay wrote:
> What does the `b` stand for in the instruction mnemonics?
> 
> Perhaps it would make sense to rename the AVR ISD nodes (such as `AVRrol` and `AVRror` to `AVRbrol` `AVRbror`).
In this mnemonic, `b` stands for byte. Line 1689 defines the `ROLWRd` pseudo-instruction, so it makes sense to call this one `ROLBRd`.

> Perhaps it would make sense to rename the AVR ISD nodes (such as AVRrol and AVRror to AVRbrol AVRbror).
I totally agree.


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:1687
+                        "rorb\t$rd",
+                        [(set i8:$rd, (AVRrol i8:$src)), (implicit SREG)]>;
+
----------------
dylanmckay wrote:
> This instruction is a right rotation, but the pattern matches on a left rotation `AVRrol`, which is also the exact same pattern as the other new instruction.
> 
> Should this be `AVRror`?
> Should this be AVRror?

Yes. I will fix this.


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:1699
                     "ror\t$rd",
                     [(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]>;
 
----------------
dylanmckay wrote:
> The patterns for `RORRd` and co still exist. Should these be removed so that we never ISel them by default?
That sounds like a good idea. The pattern, is that just the `[(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]>` part?


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

https://reviews.llvm.org/D60365