Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[AVR] Fix rotate instructions

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 ?

dylanmckay accepted this revision.Jun 28 2021, 5:24 AM
This revision is now accepted and ready to land.Jun 28 2021, 5:24 AM
This revision was automatically updated to reflect the committed changes.