This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Fix codegen for rotate instructions
ClosedPublic

Authored by dsprenkels on Apr 6 2019, 11:17 AM.

Details

Summary
This patch introduces the ROLBRd and RORBRd pseudo-instructions,
which implemenent the "traditional" rotate operations; instead of
the AVR rotate instructions that use the carry bit.

The code is not optimized at all. Especially when dealing with
loops of rotate instructions, this codegen should be improved some
day.

Related bug: 41358 https://bugs.llvm.org/show_bug.cgi?id=41358

Note: This is my first submitted patch.

Diff Detail

Repository
rL LLVM

Event Timeline

dsprenkels created this revision.Apr 6 2019, 11:17 AM
dsprenkels edited the summary of this revision. (Show Details)Apr 6 2019, 11:19 AM
dylanmckay added inline comments.May 16 2019, 2:06 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1273

This variable is called ZeroReg, and reading the code my inference is that the ZeroReg contains the register number of the zero-register, r1, which is a register that, according to the calling convention, is always guaranteed to contain the value 0.

I recommend either modifying the logic to use the dedicated zero register instead of a scavenged temporary, or renaming the variable from ZeroReg to something like TempRegWithZero.

1319

Same here re. dedicated zero register.

dylanmckay requested changes to this revision.May 16 2019, 2:06 AM
This revision now requires changes to proceed.May 16 2019, 2:06 AM

Thank you for the feedback. I will try to update the patch code this weekend.

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

When I wrote the patch I hadn't yet found that r1 is always zero. I will update the assembly to:

add r16, r16
adc r16, r1
1319

Here, I'd like the keep the variable's name TmpReg, as its value is not exactly zero after executing the intstructions.

dsprenkels removed a reviewer: shepmaster.

Updates:

  • Use the temporary registers r1 and r0, instead of scavenging spare registers.
    • And update the test case.
  • Add the definition const unsigned ZERO_REGISTER = AVR::R1 for future use.
dsprenkels marked 2 inline comments as done.May 19 2019, 3:06 AM
dylanmckay added inline comments.Jun 1 2019, 4:27 AM
llvm/lib/Target/AVR/AVRInstrInfo.td
1682

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

1687

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?

1699

The patterns for RORRd and co still exist. Should these be removed so that we never ISel them by default?

dsprenkels marked 3 inline comments as done.Jun 1 2019, 5:59 AM
dsprenkels added inline comments.
llvm/lib/Target/AVR/AVRInstrInfo.td
1682

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.

1687

Should this be AVRror?

Yes. I will fix this.

1699

That sounds like a good idea. The pattern, is that just the [(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]> part?

dsprenkels updated this revision to Diff 202591.Jun 2 2019, 3:33 AM

Removed the pattern for RORRd.

dsprenkels marked an inline comment as done.Jun 2 2019, 3:37 AM
dsprenkels added inline comments.
llvm/lib/Target/AVR/AVRInstrInfo.td
1682
Perhaps it would make sense to rename the AVR ISD nodes (such as AVRrol and AVRror to AVRbrol AVRbror).

I totally agree.

I refrained from doing this. I am not really sure which instructions should get the b suffix. If we add the suffix to the rotate instructions, should we also add them to shift instructions, etc. I hope we could postpone this improvement to another patch.

Jim added a reviewer: Jim.Jun 2 2019, 10:58 PM
dylanmckay accepted this revision.Dec 16 2019, 2:07 AM

Approved, sorry for the super long turn around on this @dsprenkels, I wanted to get an simulated integration test passing before I landed this and I finally worked out all the kinks.

Nice patch, thanks for the hard work!

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

Sounds good to me @dsprenkels

This revision is now accepted and ready to land.Dec 16 2019, 2:07 AM

Awesome. Thanks for reviewing!

I don't have any push permissions. Can you (or somebody else) commit this patch?

I will likely start on the renaming of the instructions next, (see

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

), and peephole optimization for shifts (using the swap instruction etc.).

This revision was automatically updated to reflect the committed changes.