Skip to content

Conversation

@aarongreig
Copy link
Contributor

No description provided.

@github-actions github-actions bot added loader Loader related feature/bug common Changes or additions to common utilities images UR images specification Changes or additions to the specification experimental Experimental feature additions/changes/specification command-buffer Command Buffer feature addition/changes/specification sanitizer Sanitizer layer issues/changes/specification labels Jun 14, 2024

namespace ur_mock_layer {

struct dummy_handle_t_ {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't hooked up the storage and data members of this yet, I'm kind of hoping that when it comes to porting the particular sycl unittests that were using them I can achieve the same effect with callbacks defined in the test rather than default behavior in our layer. I've left them in for now in case that isn't so.

@aarongreig aarongreig force-pushed the aaron/urCallbackLayer branch from 0722219 to 251fb91 Compare June 14, 2024 15:07
@aarongreig
Copy link
Contributor Author

I'm going to leave this in draft until I've ported most/all of the PI unittests just to make sure our initial version satisfies all the requirements there, but this is a complete PR with documentation and testing for the new layer. It isn't urgent since it'll take me a bit to port those tests I mentioned but I'd appreciate some feedback on the interface, scripting and docs @pbalcer

@pbalcer pbalcer self-requested a review June 14, 2024 15:10
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

A general comment: if we focus on the mocking use-case instead of creating a generic callback mechanism, it might be easier to instead create a mock adapter, with its own include header that the application would use to modify its behavior. We could even modify the existing null adapter for this purpose.

A side note - did you considering just using something existing like using -Wl,--wrap for the mocked tests?


virtual std::vector<std::string> getNames() const = 0;
virtual bool isAvailable() const = 0;
virtual ur_result_t init(ur_dditable_t *dditable,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sort of ugly, and will get worse the more layers need some state from the loader. Ideally we'd have a generic abstraction here that would allow the layer to somehow "collect" loader state, but that's probably an abstraction too far. Here's a simpler thought:

struct {
  layer_type type;
  union {
    struct {
      codeloc codeloc;
    } tracing;
    struct {
      callbacks callbacks;
    } callbacks;
  }
}

Or, even simpler, we can drop the union/layer_type entirely, and just pass this data as a struct - which would eliminate the need to update all the layers whenever this changes.

@aarongreig
Copy link
Contributor Author

A general comment: if we focus on the mocking use-case instead of creating a generic callback mechanism, it might be easier to instead create a mock adapter, with its own include header that the application would use to modify its behavior. We could even modify the existing null adapter for this purpose.

A side note - did you considering just using something existing like using -Wl,--wrap for the mocked tests?

I actually didn't know about --wrap, I think it could work with some clever cmake but at this point I don't really want to spend the time to try and get a whole new approach working.

I have hit some stuff while porting tests that looks like it's going to necessitate a mock header from the UR side being included there.. the only reason this is still done in a layer is that it started as the generic callback layer before I realised we'd need to focus in on the mock element so I might need to look at moving the mock stuff into the null adapter (I think the only thing we use that for in its current state is layer testing right?)

@pbalcer
Copy link
Contributor

pbalcer commented Jun 17, 2024

I think the only thing we use that for in its current state is layer testing right?

yes.

at this point I don't really want to spend the time to try and get a whole new approach working.

My experience is that --wrap is a bit fiddly, and given that you've invested so much time in this approach, sticking with it is probably the right choice.

@aarongreig
Copy link
Contributor Author

closed in favour of #1780

@aarongreig aarongreig closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command-buffer Command Buffer feature addition/changes/specification common Changes or additions to common utilities experimental Experimental feature additions/changes/specification images UR images loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification specification Changes or additions to the specification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants