> On 23 Sep 2025, at 12:55, Kyrylo Tkachov <[email protected]> wrote:
> 
> 
> 
>> On 23 Sep 2025, at 12:34, Tamar Christina <[email protected]> wrote:
>> 
>> Hi Jennifer
>> 
>>>> However the vast majority of these are 1, except for 2 entries
>>>> 
>>>>> +
>>>>> +    case NEOVERSEV2_DISPATCH_V_V13:
>>>>> +      constraints.safe_push (std::make_pair (7, 2));  /* v pipelines (2 
>>>>> slots).  */
>>>>> +      break;
>>>>> +
>>>> 
>>>> like this one.  How about dropping the std::pair and instead just adding 
>>>> the element twice to
>>>> the constaints list.
>>>> 
>>>>  case NEOVERSEV2_DISPATCH_V_V13:
>>>>    constraints.safe_push (7);  /* v pipelines.  */
>>>>    constraints.safe_push (7);  /* v pipelines.  */
>>>>    break;
>>>> 
>>>> That would simplify the code in patch 2 slightly too.
>>> Using elements multiple times makes the function 
>>> dispatch_window::fits_window more complicated because it would have to keep 
>>> track of non-unique constraints to give a correct decision. Also, other 
>>> CPUs might have more multiple-slots constraints, e.g. for reduction 
>>> instructions or vector instructions that use multiple registers.
>>> So, currently I left it with pairs.
>>> What do you think?
>> 
>> Ah, I see, it's because the fits_window would otherwise become quadratic in 
>> the current setup.
>> 
>> I've done some internal runs and haven't noticed anything regressions on 
>> workloads we track.
>> I think there are a number of improvements we can make later to this that 
>> should help scalar
>> and Adv. SIMD.
>> 
>> But for now, the patch series is OK unless anyone has any objections.
> 
> Yes, they’re fine with me too (I’ve reviewed various iterations of them 
> internally).
> Thanks for the extra evaluation.
> Kyrill
> 
Thanks, I pushed the patch series to master: 41c95d5e53e..0088e4a419e
And thank you, Tamar, for helping with performance evaluation.
Jennifer
> 
>> 
>> Thanks,
>> Tamar
>> 
>>>> 
>>>> I think we should also use an enum here (just use implicit conversion to 
>>>> cast the enum back to int)
>>>> such that the numbers mean something.
>>>> 
>>>>  case NEOVERSEV2_DISPATCH_V_V13:
>>>>    constraints.safe_push (V_PIPE);  /* v pipelines.  */
>>>>    constraints.safe_push (V_PIPE);  /* v pipelines.  */
>>>>    break;
>>> Done.
>>>> 
>>>> Aside from these I think we're mostly there.
>>>> 
>>>> Thanks,
>>>> Tamar


Reply via email to