User Details
- User Since
- Jul 12 2015, 1:48 PM (300 w, 1 d)
Sat, Mar 27
Thanks @fhahn for moving this forward!
Have mostly minor nits.
Jan 8 2021
Jan 5 2021
This looks good to me, thanks!
ok, please land independent changes separately, thanks!
(how) are the VPPredInstPHIRecipe changes related to the introduction of getLiveInIRValue()?
Good step towards representing more Values, and in particular recording InductionDescriptor information, in VPlan!
This currently requires giving access to VPValue to those classes, so we
can call the protected constructor. But most of them can be removed in
the future, once the remaining single value def recipes inherit from
VPValue.
Dec 29 2020
Good catch and fix! Minor post-commit comments.
Dec 24 2020
Dec 22 2020
This looks fine to me, thanks; would be good to get @fhahn approval too.
Dec 21 2020
Nice leverage of requiresScalarEpilogue!
Looks good to me, adding some minor comments.
Dec 12 2020
This looks good to me, adding some minor comments. Thanks!
Nov 24 2020
Nov 15 2020
This looks good to me, thanks!
Nov 14 2020
Nov 13 2020
Looks good to me, adding minor comments, thanks for pushing this forward!
Nov 12 2020
Extending the recipe abstraction to support def/use relations is an important next step forward for VPlan, thanks for pushing this momentum forward!
Oct 12 2020
Oct 9 2020
Sep 27 2020
Sep 22 2020
This looks good to me, thanks!
Sep 21 2020
This looks good to me, as part of the effort to support VPlan def-use modeling and traversals.
Note that top-down traversal from VPValue to its VPUsers, will now need to check if each VPUser isa single-def recipe/VPInstruction in order to continue downwards to its VPUsers, etc., facilitating multi-def recipes.
Jul 12 2020
Good catch!
This looks good to me, thanks! with last couple of nits.
Jul 9 2020
Jul 7 2020
Jul 6 2020
Jun 13 2020
Jun 7 2020
Looks good to me, thanks for following-up on this suggestion from D79976.
Jun 5 2020
The proposed RecurrenceDescriptor::getReductionOpChain() method identifies horizontal reductions, a subset of the vertical reductions identified by RecurrenceDescriptor::isReductionPHI(). Being two totally independent methods, it's unclear what the latter supports that the former does not. Would it be better to have isReductionPHI() also take care of recognizing horizontal reductions, and record them as in CastsToIgnore? See also TODO commented inline.
Jun 2 2020
Jun 1 2020
Thanks!
May 30 2020
Alternatively we could force the computed max VF to the next-lowest power-of-2
May 26 2020
Looks good to me. Added a minor nit.
May 25 2020
May 24 2020
May 22 2020
May 20 2020
May 19 2020
This looks good to me, thanks! Please wait a day or so if @fhahn has further comments.
May 18 2020
Perhaps clearer to introduce getMask() as in other recipes, and check if it returns null?
May 17 2020
May 15 2020
Thanks for taking care of this, only a couple of nits.
May 13 2020
There are indeed loops which can be vectorized only with a scalar remainder loop, i.e., w/o foldTail. But the tests attached could easily be made an exception: LV handles induction variables that are live-out by pre-computing their values in the pre-header. So foldTail should work fine in their presence, provided the live-out value is computed using the original trip-count rather than the one foldTail rounds up.
May 12 2020
[snip]
Right, VPWidenCanonicalIVRecipe::execute() also needs to treat VF==1 differently.
I looked at that, too. It still gives us the assert at a different location. We will need a little more work to do.
The above implies setting both VStart to CanonicalIV instead of splatting, and VStep to ConstantInt::set(STy, Part) instead of ConstantVector::get(Indices), when VF==1. Would doing so pass all your tests?
May 11 2020
[snip]
Below is an example to demonstrate that setting VTCMO to TCMO when State->VF == 1 will not help in the case of a loop of VF 1 having a vector loop-bound.
[snip]
In this case, the operand[0] (which is %vec.iv) has type <1 x i64>. The loop-bound, however, will get the type as **i64** instead of the expected **<i64 2305843009213693951>** .
May 10 2020
[snip]
In this example, the operand[0] (%induction) correctly has type i64, but the loop bound (14) is of vector type <1 x i64>
There might be multiple ways to address this assert failure. I list below a few simple ones for your reference: they might or might not be a good solution at all.
- Option 1: Not to generate the icmp instructions for %induction. In the particular case of this testcase, these instructions seem to be redundant.
- Option 2: If we are to generate the icmp instructions above, can we set the BackedgeTakenCount to the State depending on the type of the first operand? In cases like this one when the first operand is not a vector type, using Value *TCMO instead of Value *VTCMO might be an option.
I will open a Bugzzila and copy its link to this page when my password reset goes through.
Thanks,
Anh
May 9 2020
This looks good to me, thanks!
Noting a minor nit.
Follow-ups could be noted in TODO's.
Thanks for breaking this up!
This analysis/utils refactoring looks good to me, thanks! Just adding some nits to clarify some presumably "add-on" parts, and possible alternatives/follow-ups.
May 6 2020
Thanks for cleaning this up!
Just adding another follow-up.
May 5 2020
Thanks for following-up on this!
Apr 28 2020
Apr 27 2020
Apr 26 2020
Given that CheckingPtrGroup and PointerCheck are taken out of RuntimePointerChecking to become externally visible, would it make sense to rename them RuntimeCheckingPtrGroup and RuntimePointerCheck?
Added test. Rebased.
Apr 25 2020
Apr 24 2020
Apr 19 2020
Thanks for following up on this! Looks good to me with a couple of nits.
Apr 18 2020
Addressed comment regarding test; rebased.