[AVR] Do not emit instructions invalid for attiny10

[aykevl]

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.

[benshi001]

llvm/lib/Target/AVR/AVRRegisterInfo.cpp

238

It seems better to be

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

And you can change to that when committing your patch.

[benshi001]

More suggestions

1. Change the title to [AVR] Avoid emitting instructions invalid on avrtiny devices

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

3. Append Updates https://github.com/llvm/llvm-project/issues/53459 at the end of your commit message.

4. When you committing, report the commit link to https://github.com/llvm/llvm-project/issues/53459 .

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

[aykevl]

In D131867#3722606, @benshi001 wrote:

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

[benshi001]

@aykevl , you are appreciated to commit it ASAP.

[aykevl]

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

[benshi001]

Ayke, you are appreciated to commit your code ASAP.

[aykevl]

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

[benshi001]

In D131867#3927386, @aykevl wrote:

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.

[benshi001]

In D131867#3927386, @aykevl wrote:

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.

[benshi001]

In D131867#3927386, @aykevl wrote:

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.

[aykevl]

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

[aykevl]

• rebased on top of the main branch (with lots of improvements and fixed as well)

[aykevl]

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

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1141–1147

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

462

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

172–183

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

238

Fixed.

llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll

43–44

Fixed the ldd!

49–60

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.

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1141–1147

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.

1544

The same suggestion as above.

1656

The same suggestion as above.

[benshi001]

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

[benshi001]

llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll

49–60

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]

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

[aykevl]

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–60

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]

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–60

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

[benshi001]

llvm/lib/Target/AVR/AVRInstrInfo.td

324

Should it be any_of rather than all_of for HasNonTinyEncoding ?

[aykevl]

llvm/lib/Target/AVR/AVRInstrInfo.td

324

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

git grep 'all_of' llvm/lib/Target/ | grep not