Skip to content

[Driver][FPGA][SYCL] Add specific timing diagnostic for FPGA AOT #3965

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 6 commits into from
Jul 7, 2021

Conversation

mdtoguchi
Copy link
Contributor

There are cases in which the generation of FPGA device code from 'aoc'
is known to have timing issues. Add a specific diagnostic from the
driver when this case is encountered. A special return code (42) is
used to inform the driver of this situation.

Additionally in this situation, we also want to allow for the compilation
to continue so the user can use the generated binary knowing the timing
issue as stated.

There are cases in which the generation of FPGA device code from 'aoc'
is known to have timing issues.  Add a specific diagnostic from the
driver when this case is encountered.  A special return code (42) is
used to inform the driver of this situation.

Additionally in this situation, we also want to allow for the compilation
to continue so the user can use the generated binary knowing the timing
issue as stated.
@mdtoguchi
Copy link
Contributor Author

Only aoc knows when this situation occurs - any suggestion how to handle testing on our side? I will ask the PSG folks if there is a specific code sequence we can use to reproduce reliably.

@mdtoguchi mdtoguchi marked this pull request as ready for review June 26, 2021 18:42
@mdtoguchi mdtoguchi requested a review from AlexeySachkov as a code owner June 26, 2021 18:42
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 approach LGTM overall, just a couple stylistic comments

AlexeySachkov
AlexeySachkov previously approved these changes Jun 28, 2021
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

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

llvm-foreach part LGTM. However, it would be great to have a test for error code preserving mechanism

Use separate DenseMap for exit behaviors
Update names and usage model to avoid double negatives
AGindinson
AGindinson previously approved these changes Jun 29, 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.

LGTM.

@mdtoguchi mdtoguchi requested a review from AlexeySachkov June 29, 2021 15:46
@mdtoguchi
Copy link
Contributor Author

@AlexeySachkov, can you take another look?

@AGindinson
Copy link
Contributor

@bader, can this be merged without llvm-foreach owners' approval, given that @AlexeySachkov and @Fznamznon are unavailable and have granted an approval previously with a minor (now-addressed) comment?

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

llvm-foreach changes look good to me, but one comment update seems to be unintentional. Right?

@bader bader merged commit c69a311 into intel:sycl Jul 7, 2021
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