- User Since
- Jul 12 2015, 1:48 PM (270 w, 6 d)
Jul 12 2020
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
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
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
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.
In this case, the operand (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
In this example, the operand (%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.
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.
Yes, that's indeed better, thanks!
Apr 17 2020
This looks good to me, thanks!
Sure, the updated summary's fine :-).
Apr 16 2020
Clarifying the summary a bit:
Apr 15 2020
Apr 14 2020
Looks good to me, thanks!
Apr 13 2020
Thanks for cleaning this up! Final couple of nits.
Apr 12 2020
Yes, this follows D77972 naturally :-)
Thanks for this cleanup!
Apr 10 2020
Apr 9 2020
Thanks for cleaning this up. There are additional VPlan classes in other files with such a redundant 'private'; best keep them all consistent.
Apr 8 2020
Address comments, use CanonicalIV instead of overloading the original PrimaryInduction term, update some comments.