Add basic xmethod impl for accessor.operator[]

[mihails-strasuns-intel]

Intended to address templated function/operator debugging problem in
GDB. The added script is the currently used implementation by our gdb

• it only covers single-index accessor.operator[] with multiple
  dimensions, that may be extended in the future depending on user
  experience requirements.

It is much desirable to distribute this as part of compiler package
because it depends on private implementation details of SYCL headers and
must always be strictly in sync.

Signed-off-by: Mihails Strasuns [email protected]

[romanovvlad]

Intended to address templated function/operator debugging problem in
GDB. The added script is the currently used implementation by gdb-oneapi

* it only covers single-index `accessor.operator[]` with multiple
  dimensions, that may be extended in the future depending on user
  experience requirements.

It is much desirable to distribute this as part of compiler package
because it depends on private implementation details of SYCL headers and
must always be strictly in sync.

Signed-off-by: Mihails Strasuns [email protected]

Who we can test that we are in sync with the script?

[mihails-strasuns-intel]

To simplify dependencies I think it should be possible to load this with upstream 8.3 GDB and test with a host device. This is one of snippets we use in the debugger test suite:

struct user_type
{
  int x;
};

int
main (int argc, char *argv[])
{
  user_type data_in[2][2][2]
      = { { { 1, 2 }, { 3, 4 } }, { { 5, 6 }, { 7, 8 } } };
  user_type data_out[2][2][2];

  {
    cl::sycl::queue queue;
    cl::sycl::buffer<user_type, 3> buffer_in
	= { &data_in[0][0][0], cl::sycl::range<3> { 2, 2, 2 } };
    cl::sycl::buffer<user_type, 3> buffer_out
	= { &data_out[0][0][0], cl::sycl::range<3> { 2, 2, 2 } };

    queue.submit ([&] (cl::sycl::handler &cgh)
      {
	auto input = buffer_in.get_access<cl::sycl::access::mode::read> (cgh);
	auto output = buffer_out.get_access<cl::sycl::access::mode::write> (cgh);

	cgh.single_task<class simple_kernel> ([=] ()
	  {
	    auto id = cl::sycl::id<3> (0, 0, 0);
	    output[id] = input[id];
	    user_type dummy = input[id]; /* kernel-line */
	  });
      });
  }
  return 0;
}

The test case itself is to break at the commented line and verify that
print output[id].x == input[id].x outputs true.

Any suggestions how would it be better to integrate it with the compiler tests?

[romanovvlad]

Any suggestions how would it be better to integrate it with the compiler tests?

Not sure it's good idea, but one of the option is to check that required accessor class methods and members are available in AST or IR.

[mihails-strasuns-intel]

That should cover the issue of private fields but there will still be a potential desync of index calculation logic (https://github.com/intel/llvm/pull/821/files#diff-fbcdb91c2b27b0fe5eec1ccff6432ed1R29-R34) which I was warned about.

[mihails-strasuns-intel]

Anyway, I will go with whatever approach you prefer. As long as this is not merged and we use gdb internal copy, it remains a ticking time bomb that is going to explode one release or another.

[romanovvlad]

@bader Do you have any idea how we can test it?

[mihails-strasuns-intel]

Ping

[bader]

@bader Do you have any idea how we can test it?

I don't know.
The only idea I have is to take an inspiration from lldb tests https://github.com/intel/llvm/tree/sycl/lldb/, but this probably will require some additional modifications to SYCL library tests LIT config(s).

Eventually we might place this script to https://github.com/intel/llvm/tree/sycl/lldb/scripts and test it as part of lldb, but today, it is an overkill for this project.

@mihails-strasuns-intel, are you going to be the primary maintainer of this script?

[mihails-strasuns-intel]

@mihails-strasuns-intel, are you going to be the primary maintainer of this script?

Correct.

I know nothing about the current CI setup here, but would it be that hard to simply run a shell script which calls host-installed gdb and checks the result status? We don't really need any fancy test suite facilities here, just a sanity check.

[bader]

I know nothing about the current CI setup here, but would it be that hard to simply run a shell script which calls host-installed gdb and checks the result status?

We can't rely that test system has "host-installed gdb", so we need some the logic that skips the test if required dependencies are not present.

[mihails-strasuns-intel]

But will there be at least one that is guaranteed to have it? If yes, that should be sufficient and all others can ignore the check indeed.

[bader]

But will there be at least one that is guaranteed to have it? If yes, that should be sufficient and all others can ignore the check indeed.

As of today, there are no plans to provide such guarantees.

I think we should apply Vlad's suggestion on checking SYCL accessors internal member names.
WRT multi-dimensional index linearization issue, it also can be covered by an execution test.

Also you mentioned in the description that we should distribute this script with the compiler, so I suggest adding CMake logic for deploying the script.

[mihails-strasuns-intel]

I think we should apply Vlad's suggestion on checking SYCL accessors internal member names.
WRT multi-dimensional index linearization issue, it also can be covered by an execution test.

Sure. Can you please give some directions where to look?

I suggest adding CMake logic for deploying the script.

Will something like 792a599 be sufficient?

[mihails-strasuns-intel]

Ping

[bader]

I think we should apply Vlad's suggestion on checking SYCL accessors internal member names.
WRT multi-dimensional index linearization issue, it also can be covered by an execution test.

Sure. Can you please give some directions where to look?

@romanovvlad, could you elaborate on your proposal, please?

I suggest adding CMake logic for deploying the script.

Will something like 792a599 be sufficient?

@vladimirlaz, could your confirm, please?

[romanovvlad]

I think we should apply Vlad's suggestion on checking SYCL accessors internal member names.
WRT multi-dimensional index linearization issue, it also can be covered by an execution test.

Sure. Can you please give some directions where to look?

@romanovvlad, could you elaborate on your proposal, please?

Idea is to check that AST for some simple source code that use accessor on device side contains expected structure of accessor class[es] and expected names of the fields. See clang/test/AST/pragma-attribute-cxx-subject-match-rules.cpp which does similar things.

[mihails-strasuns-intel]

Sorry for the delay, has to switch to other issues for a while :/

See clang/test/AST/pragma-attribute-cxx-subject-match-rules.cpp which does similar things

I need more help with this, as there doesn't seem to be a way to express a structured AST query with FileCheck. Consider trying to enforce presence of impl field for example:

#include <CL/sycl/accessor.hpp>                                                 
// CHECK: CXXRecordDecl {{.*}} class accessor definition                        
// CHECK: FieldDecl {{.*}} referenced impl 'detail::AccessorImplDevice<AdjustedDim>'

Is there any way to ensure that the FieldDecl is encountered within the specified CXXRecordDecl? There is CHECK-NEXT but it requires matching all lines in between, which is not feasible for real headers.

[mihails-strasuns-intel]

Ping

[bader]

Sorry for the delay, has to switch to other issues for a while :/

See clang/test/AST/pragma-attribute-cxx-subject-match-rules.cpp which does similar things

I need more help with this, as there doesn't seem to be a way to express a structured AST query with FileCheck. Consider trying to enforce presence of impl field for example:

#include <CL/sycl/accessor.hpp>                                                 
// CHECK: CXXRecordDecl {{.*}} class accessor definition                        
// CHECK: FieldDecl {{.*}} referenced impl 'detail::AccessorImplDevice<AdjustedDim>'

Is there any way to ensure that the FieldDecl is encountered within the specified CXXRecordDecl? There is CHECK-NEXT but it requires matching all lines in between, which is not feasible for real headers.

Can you use substitution blocks to associate FieldDecl with specific CXXRecordDecl?
https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-string-substitution-blocks

Another option to try:

// CHECK: CXXRecordDecl {{.*}} class accessor definition                        
// CHECK-NOT: CXXRecordDecl
// CHECK: FieldDecl {{.*}} referenced impl 'detail::AccessorImplDevice<AdjustedDim>'

[mihails-strasuns-intel]

Don't see how substitution may help here. The latter option may work in practice if you are fine with some false positives - i.e. it will wrongly fail if a nested struct definition will be added inside class accessor and check will need to be updated to expect that.

Will give it a try.

[mihails-strasuns-intel]

Done, tested with make check-sycl. Anything else?

[romanovvlad]

LGTM.
@AlexeySachkov Could you please take a look as well?

[romanovvlad]

LGTM.
@AlexeySachkov Could you please take a look as well?

@AlexeySachkov ping

[AlexeySachkov]

@AlexeySachkov Could you please take a look as well?

LGTM

[mihails-strasuns-intel]

Can please someone send me AST dump for AccessorImplDevice on Windows? That may be faster than doing dev setup on my side :)

[romanovvlad]

Can please someone send me AST dump for AccessorImplDevice on Windows? That may be faster than doing dev setup on my side :)

Not sure it will be easy to find a person who works on windows :)

[mihails-strasuns-intel]

In that case I will simply disable this test for Windows - debugger does not use xmethods for that platform and if dev setup is not common either, it will only end up being a maintenance burden.

[romanovvlad]

In that case I will simply disable this test for Windows - debugger does not use xmethods for that platform and if dev setup is not common either, it will only end up being a maintenance burden.

Ok, you can do it by adding something like this:
// UNSUPPORTED: windows

[mihails-strasuns-intel]

Updated.

[mihails-strasuns-intel]

Green!

[mihails-strasuns-intel]

Thanks!