ping?

Ping?

This is merged but wasn't closed automatically, probably because I changed the commit message.

Looks good to me.

In D142107#4067538, @arsenm wrote:

Commit message is a bit misleading, I'd expect "support address space casts" would mean also handling the not-noop cases

This code modifies the pointer register, without restoring it. The pointer register should be restored to the previous value if it is is not killed (subtract one from the pointer value). See the expansion of LDDWRdPtrQ for avrtiny for example.

Looks good to me, with just a single comment. Feel free to fix this while committing.

This works for the out instruction, but what about a regular volatile store? I think it should be changed in the same way.

@vitalybuka oops, thank you for the fix!

That's indeed a nice solution, didn't think of that!

I ran the TinyGo tests with this patch and all tests still pass, while code size is reduced by 0.75%.

In D140815#4021441, @benshi001 wrote:

Since llvm-objdump decodes all address offsets of rjmp/rcall (before linking) to 0, so you can see those zeros in llvm/test/MC/AVR/inst-rjmp.s, which are not affected.

llvm-objdump only affects address offsets of rjmp/rcall that are after linking, so we have to test them in the lld tests.

Rebased the patch and updated the test case.

Closing, this has been replaced by multiple smaller patches.

• apply review feedback

• add CHECK-NOT lines to tests
• add zeroed global to the no-copy-data.ll test

This patch was largely based on X86InstrInfo::optimizeCompareInstr in llvm/lib/Target/X86/X86InstrInfo.cpp. Of course, the X86 version is much, much larger.

• update branch relaxation tests that got changed by this patch

• rebase

• rebase

• rebase (somehow the pre-merge checks failed because of an error?)

• run clang-format

Applied all review feedback and rebased the patch.

• Special-case logical shifts of 16 bits. This fixes a number of issues: it avoids unnecessary code changes in this PR, it fixes an issue after rebasing on the main branch (as seen in the buildbot failure), and it fixes an issue I found while working on D140822.
• Apply review feedback.

Looks like 3bb5ddd1756d4573d3104f8b86d2973dbc550402 broke the pre-merge check. I'll rebase the patch.

Shouldn't this also affect llvm/test/MC/AVR/inst-rjmp.s?
In any case, I think it's better to (also) have tests in llvm/test/MC/AVR for this.

• change const DebugLoc dl to const DebugLoc &dl

Update to use opcodes instead of ShiftAmt and ArithmeticShift.

• apply review comments

Thanks, I'll fix these things before committing.

(arcanist used the last commit instead of the intended commit, restoring the diff again)

I created a series of patches to replace this one, for easier reviewing. Thank you for the thorough review!
See: D140569, D140570, D140571, D140572, D140573

(testing a no-op update with arcanist)

I think having -mcpu as an alias for -mmcu on AVR is fine, but I don't think it's very useful to be honest. The -mmcu (or -mcpu) flag is inherently specific to that particular chip anyway so compatibility is not an issue. In fact, having two flags may even introduce noise and confusion.
I wouldn't be opposed to it though if there is demand for it.

Looks good to me

Looks good to me!

LGTM

LGTM

@compnerd can you take a look at my replies?

In D138529#3970417, @benshi001 wrote:

Is it possible to break this large patch to smaller ones, in which the first one is just the skeleton (with only shiftAmount = 1 implemented as the basic situation) ? Then later ones implment each special shiftAmount case .

Thank you for taking a look! I will update the patch later with your suggestions. For now, I've updated the patch a bit with changes I made locally in the past few days.
(I will also update the patch with extra context that I forgot to add this time).

• use getZeroRegister instead of AVR::R1
• use AVR::sub_lo and AVR::sub_hi instead of numeric constants
• optimize REG_SEQUENCE

• rebase (no change in patch) - hopefully the pre-merge check now works?

I have updated the patch. You are correct about the zero register in interrupts, I must have incorrectly remembered the other behavior. Unfortunately, fixing that also meant changing NEGW and ROLB.
I still think it should be safe to omit saving/restoring R17 but I might do that in a separate patch.

• add explicit zero register to NEGW and ROLB

• fix zero register save/restore in interrupts
• also test on atmega328p