Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG, SIMD: Fix 64-bit/8-bit integer division by a scalar #20153

Merged
merged 3 commits into from Oct 25, 2021
Merged

Conversation

seiko2plus
Copy link
Member

@seiko2plus seiko2plus commented Oct 21, 2021

This pull request fixes two bugs:

  1. 64-bit integer division by a scalar for both unsigned and signed division,
    which was discovered by BUG: Bad SIMD integer // on windows in numpy 1.21.2 #20025.
    It's only limited to old versions of msvc <= 2017, due to inaccurate
    implementation of the emulated version for intrinsic _udiv128,
    which leads to producing an inaccurate multiplier.

  2. 8-bit unsigned integer division, similar to the above except it is not limited to a certain compiler
    and the inaccurate multiplier was caused due to wrong casting leads to perform quad division.
    it can be triggered if the ceil(log2(divisor)) == 8.

closes #20025, #20168

  The multiplier factor that used for integer division wasn't correctly
  calculated due inaccurate implementation of the emulated version
  of intrinsic `_udiv128`.
@seiko2plus
Copy link
Member Author

@mattip, @rgommers, @seberg, I'm sorry, I lost my focus and I made a horrible mistake, I added this branch to the main repo instead of mine. should I remove this branch now and re-create another one or just keep it until this pr gets merged?

@seberg
Copy link
Member

seberg commented Oct 21, 2021

I don't think it matters, we will just delete it later. A test might be nice, but I can add it also. Thanks for taking such a quick look Sayed!

@seberg seberg added the 09 - Backport-Candidate PRs tagged should be backported label Oct 21, 2021
@charris charris added this to the 1.21.4 release milestone Oct 21, 2021
@charris
Copy link
Member

charris commented Oct 21, 2021

@seiko2plus Could you add a test for this?

  Same as 64-bit divistion, except quad wide divison leads
  to inaccurate multiplier in certain cases when the log2 of
  the divisor overflow to zero.
@seiko2plus seiko2plus changed the title BUG, SIMD: Fix 64-bit integer divison by a scalar on MSVC <= 2017 BUG, SIMD: Fix 64-bit/8-bit integer divison by a scalar Oct 23, 2021
@seiko2plus seiko2plus changed the title BUG, SIMD: Fix 64-bit/8-bit integer divison by a scalar BUG, SIMD: Fix 64-bit/8-bit integer division by a scalar Oct 23, 2021
@seiko2plus
Copy link
Member Author

seiko2plus commented Oct 23, 2021

@seberg, @charris, I have improved the current SIMD test case to cover wide data ranges, also I have discovered another one #20168 related to uint8 division. now this pr fixes the two bugs.

@charris
Copy link
Member

charris commented Oct 25, 2021

Thanks Sayed

@charris charris deleted the issue_20025 branch October 25, 2021 21:23
@charris charris added component: SIMD Issues in SIMD (fast instruction sets) code or machinery and removed 09 - Backport-Candidate PRs tagged should be backported labels Nov 3, 2021
@charris charris removed this from the 1.21.4 release milestone Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug component: SIMD Issues in SIMD (fast instruction sets) code or machinery
Projects
None yet
3 participants