- User Since
- Oct 19 2012, 12:57 AM (418 w, 3 d)
I don't want to "take sides" and I'm only looking at this review from what it is and my own experience in the community. But looking at D83088, I see a somewhat problematic review.
Sat, Oct 24
Fri, Oct 23
Fri, Oct 16
Thu, Oct 8
@MaskRay are you happy with the latest changes? If so, please change your review. Thanks!
Thu, Oct 1
Adding @craig.topper as the owner of the x86 back-end, to check the implementation of the CodeBeads concept. I have mainly worked with RISC back-ends, so it's not my area. Craig, please feel free to add whoever you think is best to review this.
There are plenty of lint checks that need addressing. Some are false positives and those are obvious cases, but the rest should be followed.
Sep 25 2020
No problems, feel free to close, thanks!
Sep 23 2020
Sep 16 2020
Sep 15 2020
Sep 10 2020
Implementation-wise, this looks good, but I wonder if we could join minsize with this using the same (or very similar) controls.
Aug 28 2020
Aug 19 2020
I'm a bit wary of all the new asserts. The vectoriser is reasonable self-contained, and if you check for the scalable flag at entry points, you can assume safety inside.
Aug 14 2020
Aug 12 2020
I think this is a good direction, too.
Jul 31 2020
Jun 17 2020
May 6 2020
Ok, turns out the failure was a flaky test due to parallelism (CMake deps not right?), but the build finishes and the tests all pass.
May 5 2020
@rriddle, do you know what's failing in the test build?
Apr 8 2020
+1 to Chris, Mehdi and John comments. I think I get the idea, but the text is certainly not conveying that, for me.
Apr 3 2020
Oh, already merged, ignore me. :)
Hi Florian, cool work, thanks!
Mar 11 2020
Based on mailing list discussions I think it should be fine for the AVR backend to be built by default. The code owners should prepare for a higher influx of bugs and be extra-responsive for a while. Thanks!
Feb 27 2020
Parachuting here, but, for reference, this is the builder that seems to be mostly green for a while: http://lab.llvm.org:8014/builders/llvm-avr-linux
Jan 10 2020
Jan 6 2020
Jan 2 2020
Some inline comments, but otherwise, LGTM. Thanks!
Dec 24 2019
Dec 20 2019
Dec 19 2019
Dec 18 2019
Dec 11 2019
This change may conflict with your other two. How are we supposed to review them? Are they a set?
Dec 10 2019
This change introduces a number of new branches in the code. While not necessarily the hottest code, it may prove significant on short loops, especially nested loops. Without evidence in the form of benchmarks etc. it's hard to justify.
Dec 9 2019
Dec 6 2019
Dec 5 2019
Is this change inspired by a real world case? If so, how relevant / pervasive is this case?
Nov 30 2019
As far as I can see, this shouldn't introduce behavioural changes and does add some nice cleanups.
Nov 1 2019
Oct 29 2019
Oct 28 2019
Oct 27 2019
This plan has all the requirements for adding a new target (http://llvm.org/docs/DeveloperPolicy.html#new-targets), mainly: ISA document, current implementation, code owner, buildbots. You just need to make sure it's marked as *experimental*, as stated in the policy.
Oct 18 2019
Oct 9 2019
So, IIUC, this is changing tryCreateRecipe to move the interleave recipe creation to the caller, buildVPlanWithVPRecipes. The dependencies with the sink values is recorded initially, then the plans are created, then the sinks are applied and, if any, the interleave groups.
Oct 7 2019
This work was mentioned on the SVE discussion about predication, adding arm folks, just in case.
Sep 26 2019
Sep 20 2019
Sep 16 2019
Added some context comments in D67601.
For context, read_register was introduced to allow use of the "global named register" GNU extension.
Sep 4 2019
Aug 6 2019
Nope, Asan failures from the sanitizer commit.
Aug 1 2019
This is a tricky one which may vary depending on the libraries available on different systems. Which toolchain is this? Can you add more context?
Jul 30 2019
I see, makes sense. LGTM, thanks!
Jul 29 2019
I understand (and agree with) the reasoning of this patch, but wouldn't this also make it harder to test the current behaviour?
Right, ArchExtKind was supposed to be shared between Arm and AArch64, but that ship has sailed a long time ago. LGTM, thanks!
Jul 26 2019
Jul 24 2019
Jul 23 2019
Jul 19 2019
Better this way, let's keep the changes contained to one per patch. If there's interest to change that behaviour or not, it should not hold this patch, which looks good to me now. Thanks!
Jul 18 2019
Jul 5 2019
As far as I can see this is an NFC refactoring with obvious benefits to readability and extensibility. LGTM too. :)
Jul 2 2019
Adding llvm-commits for wider audience
Jun 24 2019
Jun 21 2019
Jun 17 2019
I agree with Hideki, this could be in LoopVectorize.h.
Jun 14 2019
Sorry Pavel, tough week. it's at the top of my list.
Jun 10 2019
@huntergr do you have an account on bugzilla? I couldn't CC you on that bug.
Jun 6 2019
Committed as r362736
Jun 5 2019
Much cleaner, thanks! I think we should deal with extending the functionality in a separate patch, since that's already a good localized improvement.