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

[SYCL] Upstream Merge Fixes #51

Merged
merged 15 commits into from
Jul 16, 2019

Conversation

agozillon
Copy link
Contributor

@agozillon agozillon commented Jul 13, 2019

Most of the deletions come from some old scheduler files that were not automatically removed in the initial merge.

Other than that there is the usual culprits for minor fixes, SPIR builtins, reqd-work-group-size, memory_manager tweaks etc.

The main addition comes from 2 new existing issues tests and I have documented a little bit of my initial investigation inside of: #40

And a new test script for running our tests, it's a stop-gap until someone can spend the time to work out the kinks of integrating our tests into the LLVM lit test environment (Injecting our required environment, adding an option to optionally compile them, possibly annotating some of the tests for proper usage with LLVM lit and adding the correct compiler commands to the head of each file) . But it's mostly there because I got irritated running the tests by hand and it's somewhat useful.

I also altered the sycl-xocc driver based on a bug I found with the new test script.. wasn't playing nice when compiling a file in another directory. Incremental improvements as required.

With this the tests should run but it's currently REQUIRED to have DISABLE_INFER_AS in your environment. e.g. DISABLE_INFER_AS=1

Otherwise your code will work fine for sw_emu but crash in hw_emu in a lot of cases, some generic address space pollution I think. I need to investigate the new address space inference stuff a little bit deeper. This patch is to fix the next branch so that it's use-able to a state that the master branch currently is.

@keryell @Victorlzd I'd appreciate if one of you could run the new test script on 2019.1 hw_emu and see if the tests all play nice. I cannot execute hw_emu tests from 2019.1 on my machine right now, I do most of my hw_emu tests on 2018.3 still.

The hierarchical parallelism stuff will not work right now (as we deemed it unnecessary/low priority at the moment), some SPIRV intrinsic still need SPIR equivalents implemented, mainly barriers. I have opened a new issue on this for new comers to the project as it's a good one for learning about the existing implementation without having to implement any new concepts (hopefully).

agozillon added 14 commits July 10, 2019 12:44
Somethings leftover from the transferal of Scheduler 2 to the default 
and removal of the old Scheduler.
…der the file being compiled is in

Found this to be a problem when creating a run_tests.sh script that runs 
tests using a path outside of the directory e.g. 
xocc_tests/simple_tests/some_test.cpp.

In the case of KernelPropGen you'd end up with a weird name for the 
generated .sh that includes the directory in it, not ideal as the file 
just gets dumped into tmp at the moment. And it would look in an weird 
directory because of spurious /'s.

In the case of sycl-xocc it would place files into /tmp/ but look in the 
wrong place for them inside the script.

This tweak fixes this, but it needs some TLC at some point to make sure 
it makes sense for:

1) Files from anywhere in the system
2) Files resting in the same location as the compiler is invoked from
3) Multiple parallel invocations of the script
Hopefully the last time I have to fix this for a while, it's now only in 
this one location and it's a very simple check.
This is a lazy/quick fix for now until some further discussion in 
Upstream has been had and we decide on a final direction for this 
(intel/llvm#253)
…rnel

Not a relevant change for accuracy of the test as it's host related, but 
without this it was possible to crash xocc.
This will probably change again soon, but this was a little bit of a 
problem for one of the Intel tests as the createCommandQueue complains 
that it's deprecated and kills one of the new LLVM LIT tests.
Not sure if we want this in the repository, but it's a handy script for 
automating test runs for now. Which works as a quick fix in the long 
term until we have the time and inclination to work out the changes 
required to integrate into the LLVM lit infrastructure.
The bug was related to incrementing by 1 on an accessor with non-default 
unitialized underlying data, so it was undefined behaviour.
agdce_ice.cpp - is a test that will cause an ICE for xocc HLS when it 
tries to consume the LLVM/spir-df IR kernel, I passed this onto someone 
in the HLS team to have a look at for now.

boost_hana_functor_arg.cpp - seems to be address space inference 
related, an imposed address space cast seems to be breaking the inlining 
of some boost_hana functionallity. Needs some further investigation.
@agozillon agozillon requested a review from keryell July 13, 2019 01:24
Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

That looks good.
I have just seen a few typos.

#
# Most of the tests will assert and stop the shell running right now. These
# could perhaps be changed to something like a throw that's caught and prints an
# error message.. which would perhaps be more user friendly.
Copy link
Member

Choose a reason for hiding this comment

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

.. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assertions will kill the shell script, as it sends a SIGABRT I think. But it should probably be phrased as "Most of the tests will assert when they are incorrect and fail which will stop the shell script running.."

@keryell keryell merged commit 2474f81 into triSYCL:sycl/unified/next Jul 16, 2019
@keryell
Copy link
Member

keryell commented Jul 16, 2019

Thanks a lot!

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.

2 participants