Skip to content

[SYCL] Fix StringLiteral Ctor issue from #3504. #3520

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 3 commits into from
Apr 9, 2021

Conversation

erichkeane
Copy link
Contributor

I'm not sure what causes this to not like this conversion, the error
message seems incorrect and only on certain configs of the build, but we
should fix the build anyway. This calls the StringRef pointer-size
directly, rather than having the StringLiteral type do it.

I'm not sure what causes this to not like this conversion, the error
message seems incorrect and only on certain configs of the build, but we
should fix the build anyway.  This calls the StringRef pointer-size
directly, rather than having the StringLiteral type do it.
@erichkeane
Copy link
Contributor Author

Brought up by @bader here: #3504
See the error here: https://github.com/intel/llvm/runs/2299254899?check_suite_focus=true

I can't for the life of me reproduce this, and it only seems to appear in the shared-libs compilation for some reason, and a google of the "invalid string literal" message shows that this happens rarely in similar situations. I suspect we've hit some odd bug in clang, but I wasn't able to reproduce it.

Instead, StringLiteral just calls the constexpr const char*/size_t StringRef ctor anyway, so this just calls it directly.

@bader
Copy link
Contributor

bader commented Apr 9, 2021

Just to clarify, the job that failed has two noticeable differences from other jobs: it uses clang instead of gcc as a host compiler and links LLVM libraries dynamically.
I'm not sure which of these two are important to reproduce the problem.

@erichkeane
Copy link
Contributor Author

Just to clarify, the job that failed has two noticeable differences from other jobs: it uses clang instead of gcc as a host compiler and links LLVM libraries dynamically.
I'm not sure which of these two are important to reproduce the problem.

Right, I don't think EITHER should be important. However, I presume it is the former, since this is a CFE diagnostic. This code was valid, and I was unable to get a 'poc' in godbolt, so I wonder if we released a version of Clang at one point (I note clang-12 is missing from godbolt) that miscompiles this? Either way, this should fix it.

@AaronBallman
Copy link
Contributor

Just to clarify, the job that failed has two noticeable differences from other jobs: it uses clang instead of gcc as a host compiler and links LLVM libraries dynamically.
I'm not sure which of these two are important to reproduce the problem.

Right, I don't think EITHER should be important. However, I presume it is the former, since this is a CFE diagnostic. This code was valid, and I was unable to get a 'poc' in godbolt, so I wonder if we released a version of Clang at one point (I note clang-12 is missing from godbolt) that miscompiles this? Either way, this should fix it.

I wonder if there's a "missing" using declaration to bring the StringRef constructors into StringLiteral?

@erichkeane
Copy link
Contributor Author

Just to clarify, the job that failed has two noticeable differences from other jobs: it uses clang instead of gcc as a host compiler and links LLVM libraries dynamically.
I'm not sure which of these two are important to reproduce the problem.

Right, I don't think EITHER should be important. However, I presume it is the former, since this is a CFE diagnostic. This code was valid, and I was unable to get a 'poc' in godbolt, so I wonder if we released a version of Clang at one point (I note clang-12 is missing from godbolt) that miscompiles this? Either way, this should fix it.

I wonder if there's a "missing" using declaration to bring the StringRef constructors into StringLiteral?

I wouldn't think that would matter... It isn't that there is a conflict in the build, it says that the string literal is invalid and doesn't permit the StringLiteral ctor. Also, the constructor is IN the StringLiteral type.

@AaronBallman
Copy link
Contributor

Just to clarify, the job that failed has two noticeable differences from other jobs: it uses clang instead of gcc as a host compiler and links LLVM libraries dynamically.
I'm not sure which of these two are important to reproduce the problem.

Right, I don't think EITHER should be important. However, I presume it is the former, since this is a CFE diagnostic. This code was valid, and I was unable to get a 'poc' in godbolt, so I wonder if we released a version of Clang at one point (I note clang-12 is missing from godbolt) that miscompiles this? Either way, this should fix it.

I wonder if there's a "missing" using declaration to bring the StringRef constructors into StringLiteral?

I wouldn't think that would matter... It isn't that there is a conflict in the build, it says that the string literal is invalid and doesn't permit the StringLiteral ctor. Also, the constructor is IN the StringLiteral type.

Oh shoot, so it is! Yeah, that is really weird...

AaronBallman
AaronBallman previously approved these changes Apr 9, 2021
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM aside from a commenting request.

Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM again!

@bader bader merged commit 4e0a101 into intel:sycl Apr 9, 2021
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 14, 2021
* upstream/sycl: (39 commits)
  [CI] Switch to default clang-format version. (intel#3540)
  [Driver][NFC] Cleanup some option setting for SYCL offload (intel#3542)
  [GitHub Actions] Update main branch sync schedule
  [SYCL][NFC] Fix potential namespace conflicts with PSTL in tuple.hpp (intel#3541)
  [SYCL] Bump sycl library minor version (intel#3538)
  [SYCL][CUDA] Implemented cuda_piextUSMEnqueueMemAdvise (intel#3365)
  [SYCL][FPGA] Add mutual diagnostic of max_concurrency attribute in conjunction of disable_loop_pipelining attribute (intel#3512)
  [SYCL] [MATRIX] Enable joint_matrix_load, joint_matrix_store, and joint_matrix_mad for AMX (intel#3503)
  [ESIMD] Skip rewriting functions used through function pointers (intel#3527)
  [SYCL] Fix address space for spec constants buffer (intel#3521)
  [SYCL] Correct the tablegen for checking mutually exclusive stmt attrs (intel#3519)
  [SYCL][PI][L0][NFC] Refactor setting of LastCommandEvent (intel#3528)
  [SYCL] Fix group local memory sharing issue (intel#3489)
  [SYCL][NFC] Fix post-commit failure (intel#3532)
  [SYCL][Doc] Remove extension mechanism (intel#3526)
  [SYCL] Move sycl.hpp in install directory and adjust driver to match (intel#3523)
  [SYCL][ESIMD] Update ESIMD docs to address recent user comments: (intel#3516)
  [NFCI][SYCL] Correct -fdeclare-spirv-builtins to use marshalling (intel#3515)
  [SYCL] Rework MarkDevice and children (intel#3475)
  [SYCL] Fix StringLiteral Ctor issue from intel#3504. (intel#3520)
  ...
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.

3 participants