- User Since
- Aug 18 2015, 2:21 AM (397 w, 15 h)
Dec 15 2021
Dec 1 2021
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.
Nov 30 2021
Nov 29 2021
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.
Jun 29 2021
Jun 28 2021
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
May 31 2021
Thanks for the clarification, makes sense. LGTM
May 30 2021
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
May 12 2021
Feb 9 2021
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.
Feb 4 2021
Feb 1 2021
Jan 28 2021
Jan 27 2021
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.
Jan 23 2021
Nice work, approved.
Nov 18 2020
Nice patch, cheers.
Nov 17 2020
@arichardson yup, this is good to go. Thanks for the persistence on this one.
Nov 16 2020
Looks good to me, thanks for the patch!
Looks good to me, thanks for the patch @benshi001
Oct 28 2020
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).
Oct 27 2020
Sep 30 2020
Good bugfix - thanks for the second patch!
The code is looking nice, thanks for the patch @amikhalev!
This is a great patch, thanks @couchand!
Sep 21 2020
Hey @aykevl, could you please confirm that this patch looks okay?
Aug 27 2020
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.
Aug 26 2020
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?
Aug 24 2020
@ixoo committed in 0f0be3fb8ddeca4bbcffc7b22319254c360ca24b - thanks for the patch!
Aug 11 2020
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.
Jun 23 2020
Thanks for the patch @rodrigorc, appreciate it!
Jun 19 2020
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
Jun 18 2020
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.