Page MenuHomePhabricator

Add a factory method to ConstantDataArray that allows to pass in the data as StringRef
ClosedPublic

Authored by akuegel on Jun 4 2018, 4:41 AM.

Details

Summary

This simplifies the case if we already have access to the raw data that we need to store in a ConstantDataArray.
The new factor method can also be reused for implementing the factory method that gets the data as ArrayRef.

Diff Detail

Repository
rL LLVM

Event Timeline

akuegel created this revision.Jun 4 2018, 4:41 AM

getImpl is private because it's a confusing API; it's very easy to mess up endianness, and ConstantDataArray doesn't work with arbitrary LLVM types (only i8/i16/i32/i64/float/double). This wrapper isn't really much better.

What use case isn't covered by the existing get() APIs?

We want to use it for XLA, simplifying the ConvertLiteralToIrConstant method:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/xla/service/llvm_ir/llvm_util.cc#L374
Essentially we already have a buffer that we want to store as-is, and we can compute the type we want to store based on the XLA PrimitiveType. Then we could get rid of the switch, and it would make our code easier.

Oh, you're translating from your Constant abstraction to LLVM's Constant abstraction. Makes sense.

Please rename this to getRaw or something, so it's clear it's not type-safe. And please fix the doc comment to specify what element types are allowed, and note how it deals with endianness.

akuegel updated this revision to Diff 150472.Jun 8 2018, 3:07 AM

Addressed review comments:

  • renamed method to getRaw
  • added more comments regarding supported types and endianness

Friendly ping?

bkramer accepted this revision.Jun 18 2018, 2:54 AM

I think this is fine to go in. It's not the most pretty API, but not really more unsafe than what's already there.

This revision is now accepted and ready to land.Jun 18 2018, 2:54 AM
This revision was automatically updated to reflect the committed changes.