Page MenuHomePhabricator

[AVR] Fix rotate instructions
Needs ReviewPublic

Authored by aykevl on Feb 18 2021, 7:22 AM.



This patch fixes some issues with the RORB pseudo instruction.

  • A minor issue in which the instructions were said to use the SREG, which is not true.
  • An issue with the BLD instruction, which did not have an output operand.
  • A major issue in which invalid instructions were generated. The fix also reduce RORB from 4 to 3 instructions, so it's also a small optimization.

These issues were flagged by the machine verifier.

Diff Detail

Event Timeline

aykevl created this revision.Feb 18 2021, 7:22 AM
aykevl requested review of this revision.Feb 18 2021, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2021, 7:22 AM
aykevl planned changes to this revision.Feb 18 2021, 7:26 AM

Oops this is not correct.

aykevl updated this revision to Diff 324641.Feb 18 2021, 8:01 AM
aykevl edited the summary of this revision. (Show Details)
  • match avr-gcc output (bst, ror, bld)
  • fix issue with the BLD instruction

Also, there are lint errors, I think this patch is OK if all lint errors can be fixed.


BLD has a implict SREG operand, do we need to set it as killed?


remove this comment line? It is incorrect.


use llvm/utils/ to update instruction pattern?

aykevl added inline comments.Feb 20 2021, 3:07 PM

No, the kill flag is optional. See 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).


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.


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, but perhaps in a separate (non-functional) patch.

aykevl updated this revision to Diff 325250.Feb 20 2021, 3:08 PM
  • remove misleading comment
  • adjust to fix lint warnings
benshi001 added inline comments.Feb 21 2021, 6:56 AM

Do we need to set dead/kill flags for DstReg in those buildMI ?