Skip to content

Upgrade mklml and set march with official option. - #1469

Merged
jywu-mysoft merged 1 commit into
microsoft:masterfrom
xkszltl:mkldnn
Jul 26, 2019
Merged

Upgrade mklml and set march with official option.#1469
jywu-mysoft merged 1 commit into
microsoft:masterfrom
xkszltl:mkldnn

Conversation

@xkszltl

@xkszltl xkszltl commented Jul 23, 2019

Copy link
Copy Markdown
Contributor
  1. There's formal way for setting march.
  2. Upgrade to new MKLML.

Besides, the mem patch can be drop for v1.0.0 since it's fixed in upstream.

@xkszltl
xkszltl requested a review from a team as a code owner July 23, 2019 03:46
1. There's formal way for setting march.
2. Upgrade to new MKLML.

Besides, the mem patch can be drop for v1.0.0 since it's fixed in upstream.
@xkszltl xkszltl changed the title Upgrade mkl-dnn to v0.20.1 and set march with official option. Upgrade mklml and set march with official option. Jul 23, 2019
@xkszltl

xkszltl commented Jul 23, 2019

Copy link
Copy Markdown
Contributor Author

Seems the patch is very sensitive.
Keep the mkldnn at 0.18.1 but upgrade mklml.

@jywu-mysoft

Copy link
Copy Markdown
Contributor

Seems the patch is very sensitive.
Keep the mkldnn at 0.18.1 but upgrade mklml.

the mem-patch? we can ask intel for one that is compatible with v0.20.1
@sreekanth-yalachigere , is that possible?

@xkszltl

xkszltl commented Jul 23, 2019

Copy link
Copy Markdown
Contributor Author

Actually a better way is to upgrade to v1.0.
v1.0 also remove the dependency on MKL.

@jywu-mysoft

Copy link
Copy Markdown
Contributor

Actually a better way is to upgrade to v1.0.
v1.0 also remove the dependency on MKL.

yes, that is true, but that is a bigger effort than these minor changes (due to many compatibility breaks in v1.0)
as you mention, if mkldnn_sgemm reaches perf parity with MKL, then there's no need for onnxruntime to have a separate MKLML path.

@sreekanth-yalachigere

Copy link
Copy Markdown
Contributor

mem-fix patch
We need to create new mem-fix patch for 0.21

Performance improvement from 0.21
0.20
Improved GEMM-based convolutions performance.
Improved softmax performance.
Added arbitrary eltwise fusion support in GEMM-based convolutions and inner product.

0.21
Addressed static initialization order issue in bf16 converters
Fixed out of bound memory access in LRN implementation for Intel AVX2

1.0
Yes. Upgrading to 1.0 is a bigger effort. It is not backward compatible.

@xkszltl

xkszltl commented Jul 23, 2019

Copy link
Copy Markdown
Contributor Author

Oh, I mean, just FYI so that you guys may consider about it in the planning.

Also, can someone kick off the CI build?
Is there any way for me to start it?

I'm getting some weird Thread not found cmake error on some, but not all, of my machines with this PR, just wanna double check what's going on.
image

@jywu-mysoft

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 22 pipeline(s).

@xkszltl

xkszltl commented Jul 23, 2019

Copy link
Copy Markdown
Contributor Author

All build passed.
Can someone review it?

@jywu-mysoft

Copy link
Copy Markdown
Contributor

on the surface, it looks okay. i'm just not sure about taking mklml from a different version of mkldnn than the mkldnn version we build against.
are there specific benefits we're looking for from the 0.20 mkldnn's version of mklml ?

@xkszltl

xkszltl commented Jul 23, 2019

Copy link
Copy Markdown
Contributor Author

It is a minor version update of mkl (in case you don't know mklml is created directly from mkl PIC static lib), and mkl update should be hassle free and always recommended.

The benefit is, when it's shared with other component (since it's a shared lib), there will be version inconsistency and in this case the best solution is just to use the latest.

Since mkldnn is dropping mkl dependency, I wonder if this is gonna be the last mklml release.

@snnn

snnn commented Jul 23, 2019

Copy link
Copy Markdown
Contributor

But, does it mean, whenever your product want to upgrade MKL, you'll force onnxruntime do the same thing at the same time?

@xkszltl

xkszltl commented Jul 24, 2019

Copy link
Copy Markdown
Contributor Author

But, does it mean, whenever your product want to upgrade MKL, you'll force onnxruntime do the same thing at the same time?

@snnn
No I'm not forcing, just trying to help as part of open source community.

If 2 components uses the same shared lib, and there's no versioning like .so.1, there will always be conflicts.
Since backward compatibility is much more common than "forward compatibility", the way we choose is to use the latest one (or highest version amount them) at runtime.
We don't have to update build time dependency, but if there's one, why not?

Also please understand v0.20 is MKLDNN's version, not MKLML's.
MKLML's version is 2019.05.xxxxxx, which is from MKL 2019 Update X.
There's no "mismatch" between v0.18/v0.20, it's really just a URL where they host this assert along with the latest mkldnn at that time.

list(APPEND MKLDNN_CMAKE_EXTRA_ARGS "-DARCH_OPT_FLAGS=\"\"")
# v1.0
list(APPEND MKLDNN_CMAKE_EXTRA_ARGS "-DMKLDNN_ARCH_OPT_FLAGS=\"\"")
set(MKLDNN_PATCH_COMMAND1 git apply ${CMAKE_SOURCE_DIR}/patches/mkldnn/mem-patch.cmake.patch)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just noticed this.
why is the mem-patch only applied if NOT onnxruntime_BUILD_FOR_NATIVE_MACHINE,
looks like an error. I think the patch should always be applied.
lines 68 and 70 should be moved outside the if check.
(I know this isn't introduced by your PR, but since you are touching this file, it's convenient to fix it?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problem.
I thought it's by-design...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can fix it separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants