This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Do not emit instructions invalid for attiny10
ClosedPublic

Authored by aykevl on Aug 14 2022, 3:01 PM.

Details

Summary

The attiny4/attiny5/attiny9/attiny10 have a slightly modified instruction set that drops a number of useful instructions. This patch makes sure to not emit them on these "reduced tiny" cores.

The affected instructions are:

  • lds and sts (load/store directly from data)
  • ldd and std (load/store with displacement)
  • adiw and sbiw (add/sub register pairs)
  • various other instructions that were emitted without checking whether the chip actually supports them (movw, adiw, etc)

There is a variant on lds and sts on these chips, but it can only address a limited portion of the address space and is mainly useful to load/store I/O registers (as an extension to the in and out instructions). I have not implemented it here, implementing it can be done in a separate patch.

This patch is not optimal. I'm sure it can be improved a lot. For example, we could teach the instruction selector to not select lddw/stdw instructions so that the weird pointer adjustments are not necessary. But for now I've focused just on correctness, not on code quality.

Diff Detail

Event Timeline

aykevl created this revision.Aug 14 2022, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2022, 3:01 PM
Herald added subscribers: Jim, hiraditya. · View Herald Transcript
aykevl requested review of this revision.Aug 14 2022, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2022, 3:01 PM
aykevl updated this revision to Diff 452573.Aug 14 2022, 3:18 PM
benshi001 accepted this revision.Aug 15 2022, 2:00 AM
benshi001 added inline comments.
llvm/lib/Target/AVR/AVRRegisterInfo.cpp
210

It seems better to be

int MaxOffset = STI.hasTinyEncoding() ? 0 : 62;

And you can change to that when committing your patch.

This revision is now accepted and ready to land.Aug 15 2022, 2:00 AM

More suggestions

  1. Change the title to [AVR] Avoid emitting instructions invalid on avrtiny devices
  1. Simplify the commit message to
The attiny4/attiny5/attiny9/attiny10 have a slightly modified instruction set that drops a number of useful instructions. This patch makes sure to not emit them on these "reduced tiny" cores.

The affected instructions are:

lds and sts (load/store directly from data)
ldd and std (load/store with displacement)
adiw and sbiw (add/sub register pairs)
  1. Append Updates https://github.com/llvm/llvm-project/issues/53459 at the end of your commit message.
  1. When you committing, report the commit link to https://github.com/llvm/llvm-project/issues/53459 .
  1. Mention TODOs in https://github.com/llvm/llvm-project/issues/53459 , as you pointed
There is a variant on lds and sts on these chips, but it can only address a limited portion of the address space and is mainly useful to load/store I/O registers (as an extension to the in and out instructions). I have not implemented it here, implementing it can be done in a separate patch.

This patch is not optimal. I'm sure it can be improved a lot. For example, we could teach the instruction selector to not select lddw/stdw instructions so that the weird pointer adjustments are not necessary. But for now I've focused just on correctness, not on code quality.
  1. Simplify the commit message to
The attiny4/attiny5/attiny9/attiny10 have a slightly modified instruction set that drops a number of useful instructions. This patch makes sure to not emit them on these "reduced tiny" cores.

The affected instructions are:

lds and sts (load/store directly from data)
ldd and std (load/store with displacement)
adiw and sbiw (add/sub register pairs)

Why? I think the extra information in the commit message is helpful.

I will apply the other suggestions when I commit.

@aykevl , you are appreciated to commit it ASAP.

Looks like this patch doesn't apply cleanly, I'll need to rebase it.

Ayke, you are appreciated to commit your code ASAP.

While rebasing I found a bug, see: https://reviews.llvm.org/D121672#3927385

Thanks for your information. I have made a patch to fix that.

https://reviews.llvm.org/D138125

Beside changes in the clang side, some extra work is need on the backend, and I will do that later this week.

While rebasing I found a bug, see: https://reviews.llvm.org/D121672#3927385

This bug is fully fixed in https://reviews.llvm.org/D138201, you are appreciated to have a look.

While rebasing I found a bug, see: https://reviews.llvm.org/D121672#3927385

The bug you found is fixed. I think you can merge this patch.

Working on rebasing this but there were a lot of changes.

aykevl updated this revision to Diff 477640.Nov 23 2022, 4:02 PM
aykevl edited the summary of this revision. (Show Details)
  • rebased on top of the main branch (with lots of improvements and fixed as well)

Please take another look! A lot of things have changed.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1263–1269

I've changed the behavior here slightly because that's what I did in the previous version of this patch and because it's more efficient. It's better to subtract and then add the pointer than to push, subtract, and pop the previous value.

llvm/lib/Target/AVR/AVRFrameLowering.cpp
455

I think this was a preexisting bug that simply never surfaced. It was necessary to fix to get parameter loads from the stack to work correctly.

llvm/lib/Target/AVR/AVRRegisterInfo.cpp
158–169

Instead of changing the instruction, the new code creates a new movw (or two new movs) and removes the FRMIDX instruction. This seems cleaner to me and also fixes a potential bug where the MOVRdRr instruction had an implicit SREG operand (which is weird).

210

Fixed.

llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll
43–44 ↗(On Diff #477640)

Fixed the ldd!

49–59 ↗(On Diff #477640)

The code here is really terrible but it should be correct. I hope to optimize this in a later pass with a late pass that merges pointer adjustments.

aykevl requested review of this revision.Nov 23 2022, 4:12 PM
benshi001 added inline comments.Nov 28 2022, 12:29 AM
llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
1263–1269

That's good!

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

subiw is still a pseudo instruction, it would be better to give real instructions in this comment.

1407

subiw is still a pseudo instruction, it would be better to give real instructions in this comment.

1555

The same suggestion as above.

1667

The same suggestion as above.

It would be better to mention Updates https://github.com/llvm/llvm-project/issues/53459 in your commit message.

benshi001 added inline comments.Nov 28 2022, 12:53 AM
llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll
49–59 ↗(On Diff #477640)

Is such pattern common ? Is it necessary to introduce an extra pass just for this pattern?

Since AVRTiny is a known low performance device serial, I think such pattern is OK. An extra pass will lead to longer run time.

benshi001 accepted this revision.Nov 28 2022, 1:27 AM

LGTM. Thanks. You can apply my suggestion while commiting your patch.

This revision is now accepted and ready to land.Nov 28 2022, 1:27 AM
aykevl added inline comments.Nov 28 2022, 7:41 AM
llvm/lib/Target/AVR/AVRInstrInfo.td
1326

How would you suggest to do this? Something like this?

//   ld   Rd,   P+
//   ld   Rd+1, P+
//   subi PL,    2
//   sbci PH,    0

I think the original is easier to read.

1407

Same here, but here it becomes even harder to write nice instructions. Closest I can think of is this:

//   subi PL,   lo8(-q)
//   sbci PH,   hi8(-q)
//   ld   Rd,   P+
//   ld   Rd+1, P+
//   subi PL,   lo8(q)
//   sbci PH,   hi8(q)
llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll
49–59 ↗(On Diff #477640)

Is such pattern common ? Is it necessary to introduce an extra pass just for this pattern?

I don't know. Maybe I'll try it, maybe not. But these chips are so small that every bit helps.

benshi001 added inline comments.Nov 28 2022, 5:22 PM
llvm/lib/Target/AVR/AVRInstrInfo.td
1326

Indeed, I think this form is more obvious

1407

That is fine.

llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll
49–59 ↗(On Diff #477640)

That is another issue which we can decide later. Please commit this patch.

benshi001 added inline comments.Dec 8 2022, 12:21 AM
llvm/lib/Target/AVR/AVRInstrInfo.td
315

Should it be any_of rather than all_of for HasNonTinyEncoding ?

aykevl added inline comments.Dec 22 2022, 7:42 AM
llvm/lib/Target/AVR/AVRInstrInfo.td
315

It seems like all_of is the more common form. Try:

git grep 'all_of' llvm/lib/Target/ | grep not
This revision was landed with ongoing or failed builds.Dec 22 2022, 8:05 AM
This revision was automatically updated to reflect the committed changes.