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

[DAE][SYCL] Emit MD instead of updating integration header #2258

Merged
merged 4 commits into from
Aug 6, 2020

Conversation

DenisBakhvalov
Copy link
Contributor

No description provided.

bader
bader previously approved these changes Aug 5, 2020
@bader
Copy link
Contributor

bader commented Aug 5, 2020

@kbobrovs
Copy link
Contributor

kbobrovs commented Aug 5, 2020

@kbobrovs, do you know why kmeans test fails here?
http://ci.llvm.intel.com:8010/#/builders/37/builds/2446/steps/15/logs/FAIL__SYCL__kmeans_cpp

@bader - no, I don't. Is this reproduced with this PR only? @NikitaRudenkoIntel - do you have any ideas?

#6 0x00007fdf0665842a __assert_fail_base /build/glibc-2ORdQG/glibc-2.27/assert/assert.c:89:0
 #7 0x00007fdf066584a2 (/lib/x86_64-linux-gnu/libc.so.6+0x304a2)
 #8 0x000055e8034a32e1 llvm::genx::GenXSPIRVWriterAdaptor::runOnFunction(llvm::Function&) (/localdisk2/sycl_ci/buildbot/worker/Lit_With_Cuda/llvm.obj/bin/clang-12+0x2dbd2e1)
 #9 0x000055e8034a3745 llvm::genx::GenXSPIRVWriterAdaptor::runOnModule(llvm::Module&) (/localdisk2/sycl_ci/buildbot/worker/Lit_With_Cuda/llvm.obj/bin/clang-12+0x2dbd745)

@NikitaRudenkoIntel
Copy link
Contributor

Will take a look

@bader
Copy link
Contributor

bader commented Aug 5, 2020

@DenisBakhvalov, could you apply suggestion from this comment: #2226 (comment)?

I think this patch might conflict with #2207. With this patch we apply DAE unconditionally, but there are missing parts of the implementation to make it work (e.g. integration with the runtime).
Can we temporally exclude this new pass from standard optimization pipeline until the rest of the framework is ready?

@NikitaRudenkoIntel
Copy link
Contributor

For some reason this patch breaks genx.kernels metadata. The first argument of KernelMD should be a reference to kernel. This is how it looks like without this patch:

!genx.kernels = !{!4, !7}
!4 = !{void (%struct._ZTS5Point.Point addrspace(1)*, %struct._ZTS8Centroid.Centroid addrspace(1)*, %struct._ZTS5Accum.Accum addrspace(1)*, i1, i8 addrspace(1)*)* @"_ZTSZZZ4mainENK3$_0clEbENKUlRN2cl4sycl7handlerEE_clES3_E6kMeans", !"_ZTSZZZ4mainENK3$_0clEbENKUlRN2cl4sycl7handlerEE_clES3_E6kMeans", !5, i32 0, i32 0, !5, !6}
!7 = !{void (%struct._ZTS8Centroid.Centroid addrspace(1)*, %struct._ZTS5Accum.Accum addrspace(1)*, i8 addrspace(1)*)* @"_ZTSZZZ4mainENK3$_0clEbENKUlRN2cl4sycl7handlerEE0_clES3_E16kCompCentroidPos", !"_ZTSZZZ4mainENK3$_0clEbENKUlRN2cl4sycl7handlerEE0_clES3_E16kCompCentroidPos", !8, i32 0, i32 0, !8, !9}

And this is with the patch:

!genx.kernels = !{!4, !7}
!4 = distinct !{null, !"_ZTSZZZ4mainENK3$_0clEbENKUlRN2cl4sycl7handlerEE_clES3_E6kMeans", !5, i32 0, i32 0, !5, !6}
!7 = distinct !{null, !"_ZTSZZZ4mainENK3$_0clEbENKUlRN2cl4sycl7handlerEE0_clES3_E16kCompCentroidPos", !8, i32 0, i32 0, !8, !9}

The references to kernels are missing, which eventually causes the failure.

@bader
Copy link
Contributor

bader commented Aug 5, 2020

Can we temporally exclude this new pass from standard optimization pipeline until the rest of the framework is ready?

Another option is to add a command line option to control this optimization with "off" value by default.

We cannot run DAE in ESIMD cntext since the pointers to SPIR kernel
functions are saved in !genx.kernels metadata. Because of that I
removed DAE pass from the standard pipeline and put it in the place
that is specific for SYCL passes and guarded it under a new
temporary option: '-fenable-sycl-dae'. This option is only used until
other parts of the solution are not in place.
@DenisBakhvalov
Copy link
Contributor Author

@NikitaRudenkoIntel, thanks for the analysis. It turns out that we cannot run DAE in ESIMD context since the pointers to SPIR kernel functions are saved in !genx.kernels metadata. DAE pass creates new function prototypes that break metadata.

@bader, I removed the pass from the standard pipeline and put it in the place that is specific for SYCL passes and guarded it under a new temporary option: -fenable-sycl-dae. This option is only used until other parts of the solution are not in place.

@bader
Copy link
Contributor

bader commented Aug 6, 2020

@NikitaRudenkoIntel, thanks for the analysis. It turns out that we cannot run DAE in ESIMD context since the pointers to SPIR kernel functions are saved in !genx.kernels metadata. DAE pass creates new function prototypes that break metadata.

Do I understand it correctly, that fixed version doesn't apply DAE in ESIMD mode?

@bader, I removed the pass from the standard pipeline and put it in the place that is specific for SYCL passes and guarded it under a new temporary option: -fenable-sycl-dae. This option is only used until other parts of the solution are not in place.

Thanks. I'll open an issue to enable this optimization for non-SPIR kernels when full implementation is in place.

@DenisBakhvalov
Copy link
Contributor Author

@NikitaRudenkoIntel, thanks for the analysis. It turns out that we cannot run DAE in ESIMD context since the pointers to SPIR kernel functions are saved in !genx.kernels metadata. DAE pass creates new function prototypes that break metadata.

Do I understand it correctly, that fixed version doesn't apply DAE in ESIMD mode?

Yes.

Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

FE changes LGTM. I did not check llvm/ changes

@bader bader merged commit cf10351 into intel:sycl Aug 6, 2020
jsji pushed a commit that referenced this pull request Dec 14, 2023
…fter Headers change (#2258)

* Bump SPIRV-Headers to 1c6bb2743599e6eb6f37b2969acc0aef812e32e3
* replace internal SPV_INTEL_long_composites ext with the published SPV_INTEL_long_composites
* don't rename extension for now
This closes: KhronosGroup/SPIRV-LLVM-Translator#2261

Co-authored-by: Wlodarczyk, Bertrand <bertrand.wlodarczyk@intel.com>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@0166a0f
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.

5 participants