This corrects one conclusion in my review of #1585. I can no longer comment on or edit anything in that PR's thread, so I am recording the correction here.
My review measured --feature vif wall-clock on an i9-12900K: master took 156-158 ms and #1585 took 129-136 ms, about -15%. It then concluded that vpgatherdd "is not a problem on a GDS-mitigated Intel core". That conclusion does not follow. The i9-12900K is not affected by Gather Data Sampling, and Linux reports gather_data_sampling: Not affected on it, so no GDS mitigation was active. The -15% says nothing about the older Intel cores where the microcode mitigation slows gathers down. For a gather-based change, those are the cores with the most risk.
That makes the request in the review for bench_new() calls in check_vif.c more important. check_vif.c currently has none, so checkasm cannot benchmark this function at all. The checkasm CI job has run on a Xeon Platinum 8370C (Ice Lake-SP), which is in the affected range, although the job log does not show whether that host enables the mitigation. A number from a machine where the mitigation is known to be on would settle it.
The rest of the review stands. That covers the sign-extension request for accum_num_non_log, the input-range note for log2_index_32_256, and the [static 16] MSVC issue, which MSVC 19.44 rejects with C2143 in /std:c11, /std:c17 and default mode.
This corrects one conclusion in my review of #1585. I can no longer comment on or edit anything in that PR's thread, so I am recording the correction here.
My review measured
--feature vifwall-clock on an i9-12900K: master took 156-158 ms and #1585 took 129-136 ms, about -15%. It then concluded thatvpgatherdd"is not a problem on a GDS-mitigated Intel core". That conclusion does not follow. The i9-12900K is not affected by Gather Data Sampling, and Linux reportsgather_data_sampling: Not affectedon it, so no GDS mitigation was active. The -15% says nothing about the older Intel cores where the microcode mitigation slows gathers down. For a gather-based change, those are the cores with the most risk.That makes the request in the review for
bench_new()calls incheck_vif.cmore important.check_vif.ccurrently has none, so checkasm cannot benchmark this function at all. The checkasm CI job has run on a Xeon Platinum 8370C (Ice Lake-SP), which is in the affected range, although the job log does not show whether that host enables the mitigation. A number from a machine where the mitigation is known to be on would settle it.The rest of the review stands. That covers the sign-extension request for
accum_num_non_log, the input-range note forlog2_index_32_256, and the[static 16]MSVC issue, which MSVC 19.44 rejects with C2143 in/std:c11,/std:c17and default mode.