Skip to content

[Driver][SYCL] Turn on -fsycl-dead-args-optimization by default #3004

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

Merged
merged 19 commits into from
Dec 21, 2021

Conversation

mdtoguchi
Copy link
Contributor

Turn on -fsycl-dead-args-optimization by default for -fsycl
compilations. Update tests to reflect the new behavior

@mdtoguchi mdtoguchi requested review from AGindinson and a team as code owners January 8, 2021 23:58
AGindinson
AGindinson previously approved these changes Jan 9, 2021
Copy link
Contributor

@AGindinson AGindinson left a comment

Choose a reason for hiding this comment

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

The change itself LGTM. Is there something in particular that has previously been blocking the "DAE on" default before, or is it just a leftover?

@vladimirlaz
Copy link
Contributor

Failures in Jenkins/Precommit look like real regressions and they need to be fixed before submission of the PR.

@mdtoguchi mdtoguchi force-pushed the dead-args-on-default branch from 014be95 to 5366ffa Compare February 27, 2021 01:13
@mdtoguchi mdtoguchi marked this pull request as draft June 2, 2021 20:55
@mdtoguchi mdtoguchi marked this pull request as ready for review December 6, 2021 18:46
@mdtoguchi mdtoguchi requested a review from hchilama as a code owner December 6, 2021 18:46
@mdtoguchi mdtoguchi requested a review from AGindinson December 6, 2021 18:46
Copy link
Contributor

@AGindinson AGindinson left a comment

Choose a reason for hiding this comment

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

A couple suggestions & a few clarification points.

@mdtoguchi mdtoguchi requested a review from AGindinson December 10, 2021 16:43
@mdtoguchi
Copy link
Contributor Author

@AGindinson, I think your comments have been addressed, can you take another look?

Copy link
Contributor

@AGindinson AGindinson left a comment

Choose a reason for hiding this comment

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

LGTM with the current approach

@bader
Copy link
Contributor

bader commented Dec 15, 2021

@intel/llvm-reviewers-runtime, ping.

@bader
Copy link
Contributor

bader commented Dec 16, 2021

/summary:run

1 similar comment
@mdtoguchi
Copy link
Contributor Author

/summary:run

@mdtoguchi
Copy link
Contributor Author

@bader, the failures look to be performance related. How do we proceed?

@bader
Copy link
Contributor

bader commented Dec 20, 2021

@bader, the failures look to be performance related. How do we proceed?

The failures do not seems to be blocking. I suggest we commit and investigate them separately.

@bader bader merged commit 5983dfd into intel:sycl Dec 21, 2021
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Dec 23, 2021
* upstream/sycl: (1382 commits)
  [SYCL][XPTI] Report memory allocation info from SYCL runtime (intel#5172)
  [CI] Switch labels for OCL x64 job (intel#5185)
  [SYCL] Add basic support for the generic_space address space (intel#5148)
  [CI] Update CODEOWNERS for SYCL printf support passes (intel#5199)
  [SYCL][Matrix] Enable wi_slice for joint_matrix (intel#4979)
  [SYCL][Group algorithms] Move group sort extension to experimental (intel#5169)
  [SYCL] Fix kernel bundles don't really carry kernel IDs (intel#5121)
  [SYCL] Initial printf support for non-constant AS format strings (intel#5069)
  [SYCL][NFC] Fix static code analysis concerns (intel#5189)
  [SYCL][Doc] Fix typos to fix doc build (intel#5190)
  [Driver][SYCL] Turn on -fsycl-dead-args-optimization by default (intel#3004)
  [SYCL][L0][Plugin] Add support for batching copy commands (intel#5155)
  [CI] Add cache checkout script to docker containers (intel#5184)
  [SYCL][Doc] Add HIP backend to the filter selector (intel#5176)
  [Doc] Add documentation for Docker images (intel#4778)
  [LIBCLC] Add functionality for in-kernel asserts for CUDA backend (intel#5174)
  Force opt to use new pass manager in exponential-deferred-inlining test after a8c2ba1
  [SYCL] Add vec and marray support to known_identity type trait (intel#5163)
  Correctly resolve merge conflicts
  Update SPV_INTEL_hw_thread_queries to rev 2
  ...
jsji pushed a commit that referenced this pull request Feb 25, 2025
Currently, when reverse translating, we hardcode either 0 or
`SPIRAS_Private` as the address space for `alloca`s. This is not ideal
as it creates tight coupling with the current AS mappings. Furthermore,
it is not necessary, as the DataLayout holds the AllocaAS, which is what
we need. Hence, this patch changes all hardcoded callsites where we
create an `AllocaInst` to use the DataLayout specified AS. It's NFC as
it does not modify existing behaviour.

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