Skip to content

Conversation

@lbushi25
Copy link
Contributor

@lbushi25 lbushi25 commented Nov 4, 2025

There is a bug in my earlier PR #20484. When encountering an indirect call to a parallel_for_work_item function, the range of instructions that will be subjected to the lowering pass must be closed, whereas the current code simply skips over this instruction as its iterating the instructions of the basic block which effectively keeps extending the range. There is an E2E test that reflects this bug where it used to hang before this fix but so far I'm not aware of an LLVM-IR snippet of code that triggers it so for the moment, I'm just adding a SYCL-level test.

@lbushi25 lbushi25 marked this pull request as ready for review November 4, 2025 16:24
@lbushi25 lbushi25 requested review from a team as code owners November 4, 2025 16:24
@maksimsab
Copy link
Contributor

Is it possible to extract LLVM snippet from the E2E test? Bypassing LIT test modification significantly complicates later upstreaming.

@lbushi25
Copy link
Contributor Author

lbushi25 commented Nov 13, 2025

Is it possible to extract LLVM snippet from the E2E test? Bypassing LIT test modification significantly complicates later upstreaming.

I haven't been able to find an LLVM snipper to trigger this. This resulted from an high level client test failure whereas all the LLVM IR was passing. I think for now let's go ahead with the merge and I'll keep looking into it.

Aside from this, there is a test already verifying this pass's output in pfwg_and_pfwii.ll but as I said it doesn't seem to capture this bug.

@github-actions
Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

1 similar comment
@github-actions
Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

@lbushi25
Copy link
Contributor Author

@maksimsab Please have a look at my comment above. Can we go ahead with the merge here?

@github-actions
Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

@maksimsab
Copy link
Contributor

Aside from this, there is a test already verifying this pass's output in pfwg_and_pfwii.ll but as I said it doesn't seem to capture this bug.

Why do we need this change in test if it doesn't capture the bug?

@lbushi25
Copy link
Contributor Author

The latest outstanding comment was settled offline, hence the subsequent approval.
@intel/llvm-gatekeepers Can we merge this please?

@sarnex sarnex merged commit 1977533 into intel:sycl Nov 20, 2025
38 of 43 checks passed
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