Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Improve Bmi2.MultiplyNoFlags codegen #22047

Closed
wants to merge 2 commits into from
Closed

Improve Bmi2.MultiplyNoFlags codegen #22047

wants to merge 2 commits into from

Conversation

pentp
Copy link

@pentp pentp commented Jan 17, 2019

I went with the magic load route in lowering. It works out quite well (codegen is ideal, if a local var is used for the low result). Fixes #21899.

@mikedn, @fiigii, @CarolEidt do you think this might be acceptable, as all changes are limited to BMI specific functions?

@fiigii
Copy link

fiigii commented Jan 18, 2019

All the MultiplyNoFlags tests failed. Could you please address them at first?

@pentp
Copy link
Author

pentp commented Jan 30, 2019

@dotnet-bot test Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness

@pentp
Copy link
Author

pentp commented Jan 30, 2019

@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness

1 similar comment
@pentp
Copy link
Author

pentp commented Jan 30, 2019

@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness

@pentp
Copy link
Author

pentp commented Jan 30, 2019

Fixed a problem with tier0, this is ready for review now.

@pentp
Copy link
Author

pentp commented Feb 1, 2019

@fiigii could you take a look at this?

@fiigii
Copy link

fiigii commented Feb 1, 2019

I will take a look tonight.
@mikedn @CarolEidt PTAL

@mikedn
Copy link

mikedn commented Feb 1, 2019

I'd wait for @CarolEidt's feedback before going further with this. The "magic" way wasn't exactly the favored approach.

@tannergooding
Copy link
Member

It's likely also worth keeping in mind that there are other APIs (such as Math.DivRem) that would possibly benefit from whatever optimization we decide to do here (whether it is something like this as an interim solution, or something that isn't so "magic" as a more permanent solution).

@pentp
Copy link
Author

pentp commented Feb 1, 2019

I explored the MultiRegOp path used in 32-bit mode also, but that looked more complicated and most importantly, would not be pay-for-play as this magic load solution.

@pentp
Copy link
Author

pentp commented Feb 11, 2019

@CarolEidt could you take a look at this? I know this solution feels like a hack, but at least it's pretty contained and I'd like to use MULX in decimal math, but in its current form it's not really usable because of the memory accesses.

@CarolEidt
Copy link

I'm still not keen on this approach, though it is cleaner than I feared.
It will take me some time to get to this, but the review would go faster if there were more detailed comments about what's being done at each stage (and they will be needed if/when this is merged in any case). So if you could add some comments that would be great. Thanks!

@maryamariyan
Copy link
Member

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your coreclr repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

@BruceForstall
Copy link
Member

Given the lack of activity in this PR, I'm going to close it. Feel free to re-open if work resumes. Note that the repo will move soon, so it would be best to wait until the new repo opens to create a new PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bmi2.MultiplyNoFlags issues
8 participants