[SYCL-WEB] Fix test failures in address-space-conversions

[jsji]

Per @Naghasan
The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

1. Add attribute((sycl_device)) to so that the IR won't be empty.
2. Replace CHECK with CHECK-DAG because of the function order
3. Replace dso_local to {{*}} because of linkage
4. Replace declaration of tmpl to empty define tmpl..{} to avoid compilation error.

[jsji]

@mmoadeli Please have a look? It would be great if these changes can be upstream to llorg so that we don't need to resolve the conflict in sycl pulldown. Thanks.

[elizabethandrews]

I don't think you should be changing the CHECKs to CHECK_DAG here. What is causing the ordering to get messed up? Why you need to add attribute((sycl_device)). How did this pass before? What commit caused these tests to start failing?

[mmoadeli]

I don't think you should be changing the CHECKs to CHECK_DAG here. What is causing the ordering to get messed up? Why you need to add attribute((sycl_device)). How did this pass before? What commit caused these tests to start failing?

I agree with @elizabethandrews that simply replacing the checks with check_dag may not resolve the issue. The tests currently succeed upstream, indicating that the problem lies elsewhere. Interestingly, I've been tasked for substituting the check_dags with checks recently.

My impression is that a merge of upstream to intel/llvm has shown the issue.

[jsji]

I don't think you should be changing the CHECKs to CHECK_DAG here. What is causing the ordering to get messed up? Why you need to add attribute((sycl_device)). How did this pass before? What commit caused these tests to start failing?

The attribute((sycl_device)). was there before in sycl branch. It started to fail with @mmoadeli 's commit in https://github.com/llvm/llvm-project/commit//f54004475110bb0a4033261041594266c8296242
We were using check-dag before the commit.

I agree with @elizabethandrews that changing the checks with check_dag is not the solution. These tests all pass in upstream. In fact, I was asked to change the check_dags with checks.

Sure, then please have a look to see how we can fix this for sycl. Thanks.

[Naghasan]

The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

[jsji]

The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

Thanks @Naghasan . This explains the major differences. @mmoadeli Can I leave this to you to figure out proper solution? Thanks.

[mmoadeli]

The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

Thanks @Naghasan. With that explanation the fix seems fine to me. It also explains why they used CHECK_DAG in first place.

[mmoadeli]

The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

Thanks @Naghasan . This explains the major differences. @mmoadeli Can I leave this to you to figure out proper solution? Thanks.

@jsji, Considering @Naghasan's input, I believe the approach you've taken is right.

[bader]

The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

I would expect different ordering of function declarations, but not checks for instructions within functions. Do we still need to apply -DAG there?

[jsji]

The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

Thanks @Naghasan . This explains the major differences. @mmoadeli Can I leave this to you to figure out proper solution? Thanks.

@jsji, Considering @Naghasan's input, I believe the approach you've taken is right.

@elizabethandrews What is your opinion?

[jsji]

I would expect different ordering of function declarations, but not checks for instructions within functions. Do we still need to apply -DAG there?

The function declaration are after all the instructions now. So we either add -DAG to all, or we move all the function checks to AFTER instruction checks. Replacing CHECK with CHECK-DAG is more consistent with our old test file.
If we want to move all the functions instead, some of the reference in instructions will need to be updated as well.

[jsji]

Another conflict friendly fix is the use some option to control the device filtering. Do we know is there an existing option or env that can do this? SYCL_DEVICE_FILTER/ONEAPI_DEVICE_SELECTOR are runtime env, so won't take effect in compile time here.

[bader]

What @Naghasan mean by "device filtering" is source code separation into host and device parts. Upstream compiler does not do this separation yet. The device compiler in llorg compiles all functions as device code. DPC++ has no option to disable "device filtering".

[jsji]

What @Naghasan mean by "device filtering" is source code separation into host and device parts. Upstream compiler does not do this separation yet. The device compiler in llorg compiles all functions as device code. DPC++ has no option to disable "device filtering".

OK. Thanks @bader . Then I guess we have to use current solution until we upstream the "device filtering" to llorg.

[elizabethandrews]

The view in upstream is different as there is no device filtering yet (IIRC). sycl_device isn't there yet regardless (hence why it needs to be re-added) and in intel/llvm, nothing is emitted until you reach calls made by sycl_device function, causing the "reordering".

Thanks @Naghasan . This explains the major differences. @mmoadeli Can I leave this to you to figure out proper solution? Thanks.

@jsji, Considering @Naghasan's input, I believe the approach you've taken is right.

@elizabethandrews What is your opinion?

The explanation makes sense to me. I'm ok with this change. Thanks for explaining!

[jsji]

@bader Any further comments? If not, I will merge it.

[bader]

@bader Any further comments? If not, I will merge it.

No other comments.