LGTM, nice work.

By the way, as this has already been approved by one, and you rightly applied the "speak now or forever hold your peace" principle re. OpenCL, and this clearly works better from my point of view than the old code, I wouldn't want to prevent you from committing this. I have no objections to merging this in its current state. If you do merge it as-is though it would be nice if a follow up PR moves the logic into getTargetAddressSpace() where I think it better belongs

This will product correct code from an AVR perspective where (before this patch it would have failed codegen). It is consistent with how we construct function pointers in LLVM core as well.

Hey @Patryk27, very nice patch, I would not have guessed it was your first LLVM PR.

I'm going to close this in favor of https://reviews.llvm.org/D114611 . I agree with Matt's comments about there being a bit of funny business going on in this PR, the concept is being generalized in core unnecessarily and does not strictly make sense in the place I put it. I think the approach in this other patch (thanks @Patryk27) is much more sensible.

Nope, thank you!

This looks good to me, and, it is especially nice to remove the dependency on the nonexistent runtime lib functions which has been a big issue. Nice work

Thanks for the clarification, makes sense. LGTM

My apologies for the couple month latency, almost every time I've checked this patch in the last few years there isn't any new activity. It looks like there's one comment from Matt last month that still needs to be addressed (remove a piece of error handling). Once updated, I'd like to get this approved.

Very nice design and simplification, looks good to me. Thank you for the link to Ayke's very nice blog post, as well as the detailed summary about your design and the tradeoffs within. It was also helpful that you kept the refactor and the optimizations themselves in separate patches, making this a non-functional change.

Add these devices into the constant list at https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/AVR.cpp#L30 so that Clang can define the required mcu defines.

Looks good, nice and simple. Thanks @benshi001

Fix an assertion error caused by attempting to get the frame size of non-frame instructions

I've discovered an issue, this is not yet ready for review.

I have applied the code review suggestions and committed this to master in 2ccb941740e608a9cf70a7c5840497149654b0f6.

Looks good to me. Indeed, it is sad this could not be implemented at a higher level but even as is it is a great optimization.

Great patch!

Nice work, approved.

Nice patch, cheers.

@arichardson yup, this is good to go. Thanks for the persistence on this one.

Looks good to me, thanks for the patch!

Looks good to me, thanks for the patch @benshi001

Approved, thanks!

Approved, thanks for the patch!

Regarding TableGen; I would like to send an RFC to llvm-dev to come up with a proper API to expose backend-specific device-specific information and constants to LLVM frontends, as I can imagine that many backends could stand to benefit. I note that there is in general a lot of duplication in individual frontends defining information LLVM already knows about (off the top of my head, inline assembly constraints and registers is one of them but may not easily fit into the paradigm I have in mind).

Good bugfix - thanks for the second patch!

The code is looking nice, thanks for the patch @amikhalev!

This is a great patch, thanks @couchand!

Hey @aykevl, could you please confirm that this patch looks okay?

Note to the readers: A recent-ish llvm-dev discussion around this patch can be found in here whrein a consensus is found on the way forward on this patch.

I've split the backwards compat tests and AMDGPU changes out to D84345. Is the DataLayout upgrade what you meant with bitcode compatibility tests or should I be adding additional tests?

Great patch

@ixoo committed in 0f0be3fb8ddeca4bbcffc7b22319254c360ca24b - thanks for the patch!

Looks good, thanks for the patch. Do you need someone to commit this for you @ixoo?

Patch is looking really good, only suggestions are the adding a test and a couple formatting nitpicks.

Thanks for the patch @rodrigorc, appreciate it!

Committed in

Rebased version with @Sh4rK's comments fixed.

@Sh4rK your review is always welcome - I've updated the code with your comments, here's the delta containing just the fixes you've suggested.

This conflicts with master now, he's a rebased patch

Sorry, I lost track of this. Committed in 01741d6dbec11c0a0c8e610f0033831735c78d1e

Note: I don't know whether the way I've made the Z register printable is the correct way. Maybe the register should be added to the MCInst instead.

Good reproduction, works exactly as expected and indeed this value is being miscalculated.