Skip to content

Use movups instead of movdqu in block op codegen #1367

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

Closed
wants to merge 1 commit into from

Conversation

mikedn
Copy link
Contributor

@mikedn mikedn commented Jan 7, 2020

When VEX encoding is not availbale, movups encoding is one byte shorter. With VEX the two instructions have same length encoding so we can just use movups all the time.

Also fix perf score latency for movups & co., it was incorrectly set higher than movdqu's latency.

When VEX encoding is not availbale, movups encoding is one byte shorter. With VEX the two instructions have same length encoding so we can just use movups all the time.

Also fix perf score latency for movups & co., it was incorrectly set higher than movdqu's latency.
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 7, 2020
@mikedn
Copy link
Contributor Author

mikedn commented Jan 7, 2020

Somehow I missed this when improving block op codegen. Or perhaps I thought it's not relevant because it's only useful when VEX is not available and that should be rare these days. Well, unless you're crossgening…

Diff summary:

Total bytes of diff: -105703 (-0.40% of base)
    diff is an improvement.
Top file improvements by size (bytes):
      -53016 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-1.59% of base)
       -8062 : System.Private.CoreLib.dasm (-0.25% of base)
       -7991 : Microsoft.CodeAnalysis.CSharp.dasm (-0.38% of base)
       -7047 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.31% of base)
       -6368 : System.Linq.Parallel.dasm (-1.08% of base)
82 total files with size differences (82 improved, 0 regressed), 27 unchanged.
Top method improvements by size (bytes):
       -2074 (-3.36% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - CtfTraceEventSource:InitEventMap():Dictionary`2
       -1937 (-1.95% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ApplicationServerTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
       -1350 (-0.12% of base) : System.Linq.Expressions.dasm - FuncCallInstruction`3:Run(InterpretedFrame):int:this (3375 methods)
       -1256 (-2.68% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - KernelTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
       -1124 (-3.18% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ClrPrivateTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
Top method improvements by size (percentage):
          -2 (-22.22% of base) : System.Private.CoreLib.dasm - Unsafe:WriteUnaligned(byref,Guid)
          -2 (-22.22% of base) : System.Reflection.Metadata.dasm - ImportDefinitionCollection:.ctor(MemoryBlock):this
          -2 (-22.22% of base) : System.Reflection.Metadata.dasm - GuidHeap:.ctor(MemoryBlock):this
          -2 (-22.22% of base) : System.Reflection.Metadata.dasm - UserStringHeap:.ctor(MemoryBlock):this
          -2 (-20.00% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - StartStopActivity:set_ActivityID(Guid):this
13429 total methods with size differences (13429 improved, 0 regressed), 133176 unchanged.

Seems like Microsoft.Diagnostics.Tracing.TraceEvent still hasn't gave up on its plan of taking over the world by copying itself thousands of times...

@tannergooding
Copy link
Member

Are there any penalties to using movups vs movdqu on modern CPUs? What about on older CPUs where VEX isn't available?

@tannergooding
Copy link
Member

Looks like Agner's indicates no penalty on newer processors, but on some older processors it can be an additional latency between 1-4 cycles: https://www.agner.org/optimize/microarchitecture.pdf (see bypass delay for various CPUs).

@sandreenko sandreenko added the enhancement Product code improvement that does NOT require public API changes/additions label Jan 7, 2020
@sandreenko
Copy link
Contributor

@dotnet/jit-contrib

@mikedn
Copy link
Contributor Author

mikedn commented Jan 7, 2020

(see bypass delay for various CPUs).

What bypass delay? These are load/stores so they're handled by the load/store units and not by FP/int units.

@tannergooding
Copy link
Member

For example, on Core2:

The load/store unit is closely connected with the integer unit, so that there is no additional
latency when transferring data between the integer unit and the load/store unit. There is a
one clock latency when transferring data from memory (load unit) to the floating point unit,
but there is no additional latency when transferring data from the floating point unit to
memory (store unit).

On the Nehalem section (in regards to a store):

Replacing the last MOVDQA with MOVAPS has no influence on latencies,
but it may have on future processors.

On the Sandy Bridge and Ivy Bridge pipeline:

There is only rarely a bypass delay when using the wrong
type of move instruction, for example MOVAPS instead of MOVDQA.

etc...

@tannergooding
Copy link
Member

It doesn't look like something that is needed to be a consideration given that these are largely just for block copies and will generally be used with the integer pipeline in the end anyways. It was just something I was interested in as I understood their "could" be penalties in some scenarios.

@CarolEidt
Copy link
Contributor

This seems reasonable to me, especially as it is most relevant to the crossgen scenario where I believe the size is a bigger issue than at JIT time. @briansull for consideration of the PerfScore changes.
@tannergooding - I take it you are on board with this change as well?

Copy link
Contributor

@briansull briansull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dotnet dotnet deleted a comment from mikedn Jan 7, 2020
@mikedn
Copy link
Contributor Author

mikedn commented Jan 7, 2020

@briansull Care to explain why did you deleted my comment?

@briansull
Copy link
Contributor

Mike you are a valued JIT contributor and I just thought that your last comment was a bit too harsh.

I didn't really think about it too much. I was just trying to keep things peaceful here.

@richlander
Copy link
Member

I didn't read the comment (it's deleted). In general, for comments that are just a bit too harsh, we typically "hide" them instead of deleting them. It's a soft way of communicating that a comment included some text that could be offensive to someone, and then only people that are really curious see it, and everyone generally gets on with the topic. That's what we've done elsewhere and something to consider.

@mikedn mikedn closed this Jan 8, 2020
@terrajobst
Copy link
Contributor

@mikedn

@briansull Care to explain why did you deleted my comment?

Because it was completely inappropriate. Telling other contributors to not comment because "you have no clue what you're talking about" is toxic behavior. OSS is also about building & nurturing a community and that also includes involving people who still learn and have passion for the subject. We're not just here to review, merge, and maintain your PRs and ideas. While your contributions are clearly valuable, you need to accept that you're part of a larger community and need to treat others with respect.

So to be clear: just because your contributions are valuable doesn't mean you get a pass on following our Code of Conduct.

@mikedn
Copy link
Contributor Author

mikedn commented Feb 11, 2020

@terrajobst

Telling other contributors to not comment because "you have no clue what you're talking about" is toxic behavior

I don't remember what exactly the deleted comment contained but I'm pretty sure I did not tell anyone not to comment. I complained about his superficial behavior.

So to be clear: just because your contributions are valuable doesn't mean you get a pass on following our Code of Conduct.

Well, I consider his behavior to show a lack of respect for other's people work and time. As such I don't care about what the code of conduct says because anyway I don't plan to contribute more work to this OSS project. Speaking about toxic behavior...

@drieseng
Copy link
Contributor

@mikedn Could it be that it's rather a cultural difference? I used to be a Mono contributor, and once had a conflict with a maintainer. His remarks were more harsh than the ones you made, and Miguel was very diplomatic in resolving the "dispute". I honestly believe that his harsh remarks were not intentional.

I continued to be a contributer (until my work/life situation decided otherwise) as I believed in the project and I felt my contributions were appreciated.

In your situation, I think the MS maintainers handled the situation correctly (without being harsh, again this is from my point of view). There's no doubt your contributions are greatly appreciated; this by both the MS maintainers and the community.

It would be a great loss for the .NET community if you'd stop contributing. If you consider my comment to be negative, just tell me and I'll happily remove it.

@mikedn
Copy link
Contributor Author

mikedn commented Feb 11, 2020

@drieseng

Could it be that it's rather a cultural difference?

I'm not sure what that difference might be. Nobody appreciates random/out of context quoting, straightforward answer avoidance, jumping to conclusions and whatnot. Well, some might be more tolerant to it than others but still.

There's no doubt your contributions are greatly appreciated; this by both the MS maintainers and the community.

Well, I appreciate they're appreciated but at the same time I don't really care. I did not contribute to get praise, I only contributed for the fun of it. And it stops being fun if I have to deal with nonsense. If I want to deal with nonsense I can probably find that pretty easily at work, there's no need for me to come here and ruin my evening.

@mikedn
Copy link
Contributor Author

mikedn commented Feb 11, 2020

And to be clear: as far as I'm concerned this matter is closed. I have no idea why terrajobst felt the need to comment on it after more than a month.

@terrajobst
Copy link
Contributor

@drieseng

It would be a great loss for the .NET community if you'd stop contributing. If you consider my comment to be negative, just tell me and I'll happily remove it.

I wholeheartedly agree with that. @mikedn has contributed greatly and we totally appreciate all the skills & expertise for that reason. But we need to be careful that we don't use valuable contributions as a currency to offset disrespectful behavior.

@mikedn

I have no idea why terrajobst felt the need to comment on it after more than a month.

Purely practical reasons -- I was only made aware of this issue today and I believe what I said needed to be said.

Our goal is to create an inclusive and respectful community and that includes all of us: Microsoft employees, frequent contributors, and newcomers. But these are only hollow words unless we're willing to drive change where necessary. And that includes reminding people to be respectful if their actions aren't and removing people who refuse to do so. From looking at the OSS ecosystem and listening to other communities it's clear to me that ignoring toxic behavior in the spirit of taking the high road does nothing but normalizing said toxic behavior. That's why I said what I said.

Well, I appreciate they're appreciated but at the same time I don't really care. I did not contribute to get praise, I only contributed for the fun of it.

I can very much relate to that. Thus, I'd ask you to consider that this applies to other people as well. As far as I know, the vast majority of folks working on .NET do that because they love working on this stuff.

But how much fun do you think it is to hear that someone you respect tells you that you don't know what you're are talking about?

Code reviews and PR discussions are often controversial because people care. We're all guilty of saying things that come across much harsher than we intended. Being respectful doesn't mean not making mistakes, it means unwillingness to learn from them and/or doubling down on them. And I for one am not willing to tell people to suck it up because that's just the culture we have. We generally don't have that culture internally or with our community. And the places where we do I consider it a defect that needs to be fixed.

@mikedn
Copy link
Contributor Author

mikedn commented Feb 12, 2020

@terrajobst

But how much fun do you think it is to hear that someone you respect tells you that you don't know what you're are talking about?

And if you really do not know what you are talking about then what? You shield yourself in the code of conduct and go OMG, how could people dare telling me that I'm wrong? Or perhaps you try to do better next time and avoid the not so fun part of messing up? Working for fun doesn't imply that you get to do whatever you want and that there are no consequences.

Code reviews and PR discussions are often controversial because people care.

There was no real controversy here. I asked a question and I was expecting a reasonable answer, especially considering that it wasn't me who started this. Instead of an answer I've got some gibberish that tries to pass as code review or whatever that is supposed to be.

Being respectful doesn't mean not making mistakes, it means unwillingness to learn from them and/or doubling down on them

It's exactly this unwillingness to learn from mistakes that brought us here. So now you're trying to explain to me how persistent shoot from the hip style commenting is respectful or not? Frankly I'm not sure what exactly it is. Let's just say that it is distasteful. Feel free to continue to go down this path, the only thing you'll achieve is to dig the hole deeper.

And I for one am not willing to tell people to suck it up.

Yet here you are, telling me exactly that.

@terrajobst
Copy link
Contributor

terrajobst commented Feb 12, 2020

@mikedn

Tanner acted in good faith by asking questions to the best of his understanding. Telling him that he doesn't know what he's talking about is unacceptable. First of all, it's not an insightful comment from your end. If you want other people to learn, you need to give them more information beyond "you're wrong". But more importantly it's just plain rude. You're basically sending him the signal of "you're not welcome here".

But instead of apologizing or even just letting that feedback sink in, you're now trying to turn the tables by saying you got victimized by Tanner because he asked you a technical question and that it's unreasonable from our end to ask you to be respectful in your responses to him.

You're more than welcome to continue to contribute to .NET. But I'll be clear: if we observe behavior like this in the future, we'll ultimately block you. We can't have other contributors stop engaging with us because of your behavior. While your contributions are valued, they don't warrant losing other contributors as collateral damage.

@dotnet dotnet locked as too heated and limited conversation to collaborators Feb 12, 2020
@mikedn mikedn deleted the block-op branch August 30, 2020 08:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants