Skip to content

Xrt tensor alloc#112

Merged
asuhan merged 3 commits into
masterfrom
xrt_tensor_alloc
Jan 14, 2019
Merged

Xrt tensor alloc#112
asuhan merged 3 commits into
masterfrom
xrt_tensor_alloc

Conversation

@dlibenzi
Copy link
Copy Markdown
Collaborator

I am not 100% happy with the current status.
We have a computation client interface which takes a Literal, which means we have to convert to Literal, then to Tensor.
Ideally we would want to read (to-server) and write (from-server) directly from/to an ATEN tensor buffer. So that'll will be done in a near future.
Having the computation client interface accept ATEN tensors (plus requested on-device shape+layout, for to-server) might be the obvious choice, but that's a problem on itself due to bazel building ATEN stuff.

@dlibenzi dlibenzi requested a review from asuhan January 14, 2019 02:02
Comment thread third_party/xla_client/util.h Outdated
return pointers;
}

template <typename T, typename C>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't look particularly useful outside of XRT client, maybe leave it in third_party/xla_client/xrt_computation_client.cc until it gets more users. Also, can't we use std::copy instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's a utility function. If we bury it inside a cc file, we need to remember to look there as well when we wonder of something like that already exists.
Can't use std::copy there, as this marshal different types.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm confused, std::copy works with different element types, what is the problem here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It does, but here (in the specific call site) we are narrowing from int64 to int.
It triggers narrowing warnings.
If we are OK with those, this API can turn to:

return std::vector(a.begin(), a.end());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh, heck, we have so many warnings already.
Dropped the API altogether.

MakeEquivalentTensorShape(literal.shape()));
auto tdata = tensor.tensor_data();
XLA_CHECK_EQ(tdata.size(), literal.size_bytes());
// This is an hack.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a hack

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think, since the 'h' is transparent, should be 'an'. but you can find both ways around:

https://www.merriam-webster.com/words-at-play/is-it-a-or-an

If you are picky I can change it (that comment will be going away soon anyway ;) )

@soumith
Copy link
Copy Markdown
Contributor

soumith commented Jan 14, 2019

one obvious choice would be to use dlpack conversion, i.e. add support in the computation client to accept DLPack Tensors.

dlpack is a single header C struct that we decided among PyTorch, MXNet, Chainer etc. as an agreed upon exchange format for Tensors.

PyTorch Tensors can be converted to dlpack with torch.utils.to_dlpack and torch.utils.from_dlpack with zero memcopy.

References:

@dlibenzi
Copy link
Copy Markdown
Collaborator Author

Thanks for the tip Soumith. Will take a look.
Unfortunately TF Tensor (which we need to feed the feed dictionary) does not allow the creating from borrowed pointer, but moving the re-layout logic into the XRT layer we can avoid altogether the current copy, and act directory from ATEN Tensor flat buffer, into TF Tensor buffer.

@asuhan asuhan merged commit 7722d4f into master Jan 14, 2019
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.

3 participants