- User Since
- Sep 16 2016, 11:47 AM (160 w, 2 d)
Sat, Sep 14
Aug 26 2019
Thanks, Ayal! LGTM
Aug 21 2019
Not needed for now
Not needed for now.
Aug 6 2019
Aug 5 2019
Thanks, @plotfi. Sorry about that. It passed with gcc in my local machine. I'll check with Clang.
Thanks for the quick response, Lang! I'll proceed with the commit in a while, in case somebody else has any comments.
Apr 9 2019
LGTM! Thanks, Francesco!
Mar 29 2019
Or are you saying this needs to be done explicitly for stress testing?
Mar 28 2019
Should we add a small lit test that covers the new expected behavior? Probably reusing one from D57598 with the stress testing flag would suffice.
I thought the point was to just have a VF to build a VPlan with and as it currently stands we build the same VPlans with VF = 4 or any other VF I think. With the programmatically determined VF, we would be a little closer to reality in the stress testing. As Hideki mentioned, having a constant factor for stress testing might have other benefits later on though
Yep, I agree on that we should keep the stress testing mechanism. It will be very useful to make sure that the construction and predication (and maybe other transformation) are robust enough since we can run it on loop nests that are not necessarily vectorizable.
Something important, though, is that we shouldn't use this mechanism to bypass legality or pragma simd requirements to vectorize a loop, i.e., we shouldn't use it to generate actual vector code.
What are you trying to achieve, Francesco?
Mar 14 2019
Thanks, Francesco. LGTM!
Feb 6 2019
LGTM (as you could see in my previous message :))
Feb 5 2019
Feb 4 2019
Thanks Francesco for helping us remove some of the constraints we have in VPlan native path!
I agree with the idea of using getSmallestAndWidestTypes() as a starting point whereas we don't have the proper cost model.
Jan 31 2019
Jan 30 2019
Hey Florian! Thanks a lot for taking care of this! I'm leaving a comment below. Please, let me know what you think.
Jan 11 2019
Nov 12 2018
From the VPlan point of view, LGTM. I don't have any other comments.
Oct 10 2018
Aug 22 2018
In case you missed it, there is some discussion in D50480 regarding this code. Your feedback would be appreciated.
Reverted to use the original ICmpULE extended opcode instead of detached ICmpInst. This can be revised quite easily once VPInstructions acquire any other form of modeling compares.
Aug 20 2018
Aug 17 2018
Aug 15 2018
Aug 14 2018
Aug 12 2018
Thanks, Ayal! Some comments below.
Do you see any potential issue that could make modeling this in the VPlan native path complicated once we have predication?
Aug 7 2018
Just a few comments.
Aug 6 2018
Thanks for working on this, Florian, and sorry for my delayed response. I added some initial comments. I'll come back soon.
Jul 30 2018
Jul 27 2018
Thanks, Florian! I'll go ahead with the commit!
Jul 20 2018
Thanks for the review, Florian!
I'll commit this once D48815 is approved and committed.
Jul 17 2018
Making VPLoopInfo object a member of VPlan.
Reverting GraphTraits changes and adding VPlanTestBase as friend of VPlanHCFGBuilder.
Jul 16 2018
Thanks for the comments, Florian! Addressing your comments.
Jul 12 2018
Rebasing this patch on top of D49032
Rebasing this patch on top of D49032.
Rebasing this patch on top of D49032.
Jul 9 2018
Jul 6 2018
Addressing Chandler's comments. Thanks, Chandler!
Yes, I have commit access.
Good idea! Thanks! LGTM. Just some minor comments.
Thanks, Florian! I'll commit next week.
Jul 1 2018
Jun 15 2018
Thanks a lot, Florian! LGTM!
Jun 14 2018
If no more comments, I'll proceed with the commit.
Jun 13 2018
Thanks for the rework and for your patience, Florian!
I don't see any major issues. Please, wait for Satish's approval, just in case he has any comments regarding how this would interact with his next patch.
In any case, we could always address any issues in a separate commit.
Thanks for the patch! LGTM.
I think it's ok to kept these utilities in the VPRecipeBase class for now. Eventually, if we add too many, we may want to move them to a VPInstructionUtils, same as what we are doing for VPBlockBase.
Thanks for the comments, Ayal! Please, let me know if you have any other concerns.
Jun 12 2018
Thank you both for the review!
Jun 11 2018
Jun 7 2018
Jun 4 2018
Thanks for this new version, Florian! Pretty excited to see that some inner loop tests are passing!
Satish is working on the support for outer loops in CG and we’ve been discussing your changes. We have to make sure that this patch aligns well with his next patch and also with the constraints that we stablished for this patch series and subsequent ones. Some comments in this regard:
- We think that VPInstructionsToVPRecipes is still too smart for the vplan native path and bringing too much code (from CM and Legal, for example) an recipes that we would prefer to keep away from the native path until they are properly ported. Otherwise, we’d end up having the same development limitations as we have in the inner loop path. What we had in main for the Recipe to VPInstruction transition in the native path is something as simple as the code in https://reviews.llvm.org/D44338?vs=on&id=140081&whitespace=ignore-most, where createRecipesForVPBB is naively creating only VPWidenMemoryInstructionRecipe, VPWidenRecipe and VPWidenPHIRecipe. Satish will give you more details in this regard so that this patch aligns with his.
Jun 1 2018
Thanks, Florian! Again, wait for the pending review to commit this one.
Thanks, Florian! LGTM!
Please wait until we finish the other two related reviews to commit it, in case we have to change something else.
May 31 2018
Thanks for doing this refactoring! In general, moving all this code outside of the planner makes sense to me.
create*Mask and getDecisionAndClampRange are not strictly creating recipes but I understand the dependencies.
Minor comments inline.
Thanks for revisiting this, Florian!
No major issues. Just one question below.