[Target] Allow spaces in target attributes

[Lunderberg]

Some target parameters, such as the device_name on vulkan, have spaces in them. This prevented round-trips between string and Target objects, which can occur in some cases (e.g. AOT codegen).

• Allowed for spaces in target attributes.
• Bugfix for target string vulkan -from_device=0, was missing device name from device query.

[Lunderberg]

@zxybazh Long-term, I'd like to switch to using JSON exclusively for target -> str conversions, as we had discussed, with the existing argument parsing only used for str -> target conversions. For now, since the target string is saved in autotvm's logs and I'm not sure how extensively it is used, extending the argument parsing to escape strings, in order to avoid potential breakage.

[zxybazh]

Thanks for the contribution!

Json-style target string parsing is supported as the canonical way for target creation and json export is supported using the export function (though right now the export result is not pure json, it's actually a tvm Map object). In the long term, I am in favor of using json exclusively for target -> str conversion.

Meanwhile, IIRC we are using the current str function (target->str) for compatibility with AutoTVM and Ansor (AutoScheduling). IMHO, If we want to extend the argument parsing to escape strings, I would suggest that we keep the str function the same, and work on parsing and export function first (with compatibility for str function output). Other than that, I'm not aware of any other potential breakage. I think Junru might add to that.

cc @junrushao1994 .

[Lunderberg]

Json-style target string parsing is supported as the canonical way for target creation and json export is supported using the export function (though right now the export result is not pure json, it's actually a tvm Map object). In the long term, I am in favor of using json exclusively for target -> str conversion.

Definitely agreed, long-term it's better to go through a standard format. I played around a bit with converting the tvm Map to a json string, but while it wasn't too hard to get something working for my test cases, getting it fully functional and clean felt like it would take longer than a quick edit. (And I'd probably have wanted to switch to a C++ JSON parser to avoid jumping between languages too often, which would have added its own issues.)

Meanwhile, IIRC we are using the current str function (target->str) for compatibility with AutoTVM and Ansor (AutoScheduling). IMHO, If we want to extend the argument parsing to escape strings, I would suggest that we keep the str function the same, and work on parsing and export function first (with compatibility for str function output). Other than that, I'm not aware of any other potential breakage. I think Junru might add to that.

Yeah, the compatibility was what I was most concerned about. I wanted to update the .str() method itself, rather than introducing a new function such as .json(), because that's what gets called elsewhere. The updated parser/serializer should maintain the exact same string output for attribute keys without embedded spaces, but "should" is always a bit of a dangerous word.

[zxybazh]

Definitely agreed, long-term it's better to go through a standard format. I played around a bit with converting the tvm Map to a json string, but while it wasn't too hard to get something working for my test cases, getting it fully functional and clean felt like it would take longer than a quick edit. (And I'd probably have wanted to switch to a C++ JSON parser to avoid jumping between languages too often, which would have added its own issues.)

Yes, it's more than a little work to make the Map 100% compatible to json. A c++ Json parser is ok to me but not necessary if there is nothing the current parser cannot do -- just to save time and as least the python parser is guaranteed to be correct right : )

Yeah, the compatibility was what I was most concerned about. I wanted to update the .str() method itself, rather than introducing a new function such as .json(), because that's what gets called elsewhere. The updated parser/serializer should maintain the exact same string output for attribute keys without embedded spaces, but "should" is always a bit of a dangerous word.

I think the export function is acting as .json() right now. For str it would be a large project because it's used in a lot of places in key components. I think it's worth more discussion before we make it happen : ) But definitely direction to go in the future.

[leandron]

In tvmc we faced this challenge of having substrings within target(s) (see relevant tests), one example is llvm arguments. The decision there was to go with nested quotes, similar to Python. Have you explored this possibility in here?

That would make it look like:

"vulkan -device_name='Name of GPU with spaces' -device_type=discrete"

I feel that is looks a bit more natural to type in nested quotes rather than preceding everything with a backslash. Also, I think it would be aspirational to offer a similar experience (where possible), when using API and command line.

What do you think?

[Lunderberg]

I'd be down for either, and agree that the nested quotes are an easier way to read it. Consistency with how tvmc handles spaces is also something that I want to maintain, so I'll update this PR to use the same convention.

[Lunderberg]

And updated.

[masahi]

@Lunderberg On AMD I got a different error with this PR: (UPDATE) The error below is introduced in #8602. With 8602 reverted and applying this PR, the original error I saw was gone.

  4: tvm::TargetInternal::ConstructorDispatcher(tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*)
  3: tvm::Target::Target(tvm::runtime::String const&)
  2: tvm::TargetInternal::FromString(tvm::runtime::String const&)
  1: tvm::TargetInternal::FromRawString(tvm::runtime::String const&)
  0: tvm::TargetInternal::FromConfig(std::unordered_map<tvm::runtime::String, tvm::runtime::ObjectRef, std::hash<tvm::runtime::String>, std::equal_to<tvm::runtime::String>, std::allocator<std::pair<tvm::runtime::String const, tvm::runtime::ObjectRef> > >)
  File "/home/masa/projects/dev/tvm/src/target/target.cc", line 469
TVMError: ValueError[10:18:05] /home/masa/projects/dev/tvm/src/target/target.cc:788: Expected runtime.String parameter for attribute 'device_name', but received TVMArgTypeCode(8) from device api
. Target creation from string failed: vulkan -from_device=0

[masahi]

Thanks @Lunderberg, please take a look at the issue of #8602 above.

[Lunderberg]

Thank you. I can reproduce the issue, and will have a PR shortly to resolve it. I had tested the new behavior during development, but it looks like the final version was broken on intel CPUs as well.