Page MenuHomePhabricator

[AVR] Fix rotate instructions
Needs ReviewPublic

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

Details

Summary

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.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1349

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

llvm/lib/Target/AVR/AVRInstrInfo.td
1753

remove this comment line? It is incorrect.

llvm/test/CodeGen/AVR/rot.ll
40–42

use llvm/utils/update_llc_test_checks.py to update instruction pattern?

aykevl added inline comments.Feb 20 2021, 3:07 PM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1349

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).

llvm/lib/Target/AVR/AVRInstrInfo.td
1753

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.

llvm/test/CodeGen/AVR/rot.ll
40–42

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.

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
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1348

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