Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hexagon] Rework tvm.target.hexagon() interface #8823

Merged
merged 6 commits into from
Aug 26, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 90 additions & 47 deletions python/tvm/target/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -413,79 +413,118 @@ def bifrost(model="unknown", options=None):
return Target(" ".join(["opencl"] + opts))


def hexagon(cpu_ver="v66", sim_args=None, llvm_args=None, hvx=128):
def hexagon(cpu_ver="v66", **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, we can use the following function signature so that the arguments are clear, and deprecate sim_args and llvm_args later.

Suggested change
def hexagon(cpu_ver="v66", **kwargs):
def hexagon(cpu_ver="v66", *, sim_options=None, llvm_options=None, hvx=128, sim_args=None, llvm_args=None):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @zxybazh for the detailed feedback. You are making valid points, but I think I should share more context of why the code looks like this.

In our downstream repository we have a few more options for the Hexagon target that, for one reason or another, we cannot yet make public. We have reorganized the tvm.target,hexagon() so that we don't need to change the interface every time we add a new flag---this is the reason to use the keyword arguments instead of explicit named arguments.
The code downstream has diverged from the upstream version, and we would like to keep these two close to reduce the risk of conflicts (when integrating upstream code to our downstream repo). Essentially, what this patch is, is the downstream code with the non-public parts deleted. This is why several places look like they could be simplified, but if we did simplify them upstream, it would create divergence again.
I'd like to ask that this code is accepted without removing args since the downstream code has more fields in it, and is organized around handling the contents of the target configuration represented by args.

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'll revisit the other comments a bit later. The general idea there was to create sim/llvm/etc strings from the "target configuration", which is represented by args and cpu_ver. This is why all the string-generating functions take the same arguments, even if some of these arguments aren't necessary, or are already available without parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification. It would be fine to me if you want to keep the args variable this way. Would appreciate a few lines of docs explaining the reason. Please feel free to the remove the argument related issues I proposed.

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 renamed args to config, hopefully it will better reflect the purpose of the variable.

"""Returns a Hexagon target.

Parameters
----------
cpu_ver : str
cpu_ver : str (default: "v66")
Copy link
Contributor

Choose a reason for hiding this comment

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

@kparzysz-quic is it possible to query hexagon to return some of these device specific attributes? E.g. the architecture rev, vector width, available memory.

There is some desire to add general support to the DeviceAPI to query device specific parameters to use to build a target which can then influence lowering and codegen. The Vulkan target already achieves this via a packed function, but it could be extended to support all DeviceAPI subclasses. But this will only help if the hexagon sdk supports device attribute querying. If so, we can consider revisiting the target construction after converging on a proposal for the above change to the Device API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I saw in the Vulkan code, it seems like the device information is obtained by directly querying Vulkan runtime.

In case of Hexagon, the compilation will almost always happen on a machine without any Hexagon hardware present. If anything, the situation here may be the reverse of the Vulkan case---the Target would be the definition from which DeviceAPI would be created. At execution time, if the actual hardware does not meet the criteria given by the Target, the execution could simply fail with a message.

In general, having a Hexagon phone plugged into an x86-based PC does not really make it easy to query it, as most libraries in the Hexagon SDK are built for either Hexagon itself or for ARM/AArch64. Users would normally do adb shell and look for configuration details in the files in the system.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct for the way Vulkan currently retrieves device attributes.

However the proposal would be to elevate the device attribute querying to the DeviceAPI. In this case the RPC support that already exists with the DeviceAPI would be used to query the remote DeviceAPI. The intention is that the device attributes would be queried remotely and then sent back to the local machine used for cross compiling. It could then used to build a device specific target used to inform lowering and codegen. In short, the approach should not require the device to be local to the machine doing the compilation, as indeed there are many cases when this is not practical. Do you still see technical feasibility issues with this approach?

The goal would be to improve performance and accessibility by requiring less information provided by the user of TVM in order to achieve optimal device specific code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's an interesting suggestion Chris - querying device attributes on Vulkan has helped a lot for codegen given the looseness of the Vulkan standard. In some ways I don't see this as too much of an usability burden, it's not unlike the mcpu option we have to pass in for llvm codegen when making sure to getting the best out of a given target x86 architecture.

Copy link
Contributor

Choose a reason for hiding this comment

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

My take is that the need for the user to provide the often very detailed mcpu option is the usability burden IMO.

In some SDKs the ability exists to query sufficient information to construct the mcpu option equivalent, and I'd love to see TVM be better able to use this information to produce optimal codegen automatically. Though I'm still not sure if that will be possible or helpful hexagon, just wanted to raise the discussion with @kparzysz-quic if he sees hexagon specific merit for the effort.

CPU version used for code generation. Not all allowed cpu str
will be valid, LLVM will throw an error.
sim_args : str or list of str

Recognized keyword parameters
-----------------------------
hvx : int (default: 128)
Size of HVX vector in bytes. Value of 0 disables HVX codegen.
sim_options : str or list of str (default: None)
User defined sim arguments. CPU version defaults to cpu_ver.
Otherwise, separate versions are used for codegen and sim. Not
all allowed cpu strings will be valid, simulator will throw an
error if invalid. Does not affect codegen.
llvm_args : str or list of str
llvm_options : str or list of str (default: None)
User defined compiler arguments.
hvx : int
Size of hvx register. Value of 0 indicates disabled hvx.
"""

# Some of the target parameters correspond to target kind attributes
# listed in src/target/target_kind.cc. For those parameters, their
# names follow the attribute names with the exception of '_' being used
# in place of '-'.

# Example compiler arguments
# llvm -mtriple=hexagon -mcpu=hexagonv66 -mattr=+hvxv66,+hvx-length128b

# Check for valid codegen cpu
valid_hex = ["v60", "v62", "v65", "v66", "v67", "v67t"]
valid_hex = ["v60", "v62", "v65", "v66", "v67", "v67t", "v68"]
try:
cpu_ver = cpu_ver[cpu_ver.index("v") :].lower()
assert 3 <= len(cpu_ver) <= 4
assert cpu_ver in valid_hex
except:
msg = "{} is not a valid Hexagon version\nvalid versions include {}"
raise ValueError(msg.format(cpu_ver, valid_hex)) from None

assert hvx in [0, 64, 128]
# Target configuration:
config = {
"hvx": 128,
"sim_options": None,
"llvm_options": None,
}
config.update(kwargs)

# Warn about obsolete parameter names.
if config.get("sim_args"):
msg = "The keyword parameter 'sim_args' is deprecated, use 'sim_options' instead"
warnings.warn(msg, stacklevel=2)
config.update({"sim_options": config["sim_args"]})
if config.get("llvm_args"):
msg = "The keyword parameter 'llvm_args' is deprecated, use 'llvm_options' instead"
warnings.warn(msg, stacklevel=2)
config.update({"llvm_options": config["llvm_args"]})

# LLVM target string
def create_llvm_target(cpu_ver, config):
""" Create LLVM target string. """

# Target string
def create_target(cpu_ver):
target = " -mtriple=hexagon"
mcpu = " -mcpu=hexagon" + cpu_ver
mattr = ""
# HVX enable
if hvx:
mattr = " -mattr=+hvx" + cpu_ver + ",+hvx-length" + str(hvx) + "b"
return target + mcpu + mattr

# Simulator string
def create_sim(cpu_ver, sim_args):
def validate_hvx_length(codegen_hvx, sim_args):
if sim_args and "--hvx_length" in sim_args:

# Process the options that affect target features and return the
# target feature string.
def create_target_features(config):
tfs = []
if config["hvx"] > 0:
valid_hvx = [0, 64, 128]
if not config["hvx"] in valid_hvx:
raise ValueError("Invalid hvx value, should be one of " + str(valid_hvx))
tfs += ["+hvx" + cpu_ver, "+hvx-length" + str(config["hvx"]) + "b"]
else:
tfs += ["-hvx"]
return "-mattr=" + ",".join(tfs) if tfs else ""

return target + mcpu + " " + create_target_features(config)

# Simulator options string
def create_sim_options(cpu_ver, config):
""" Create simulator option string. """

def validate_hvx_length(codegen_hvx, sim_options):
if sim_options and "--hvx_length" in sim_options:
# If --hvx_length was specified, check HVX length of sim
# vs codegen
i = sim_args.index("hvx_length") + len("hvx_length") + 1
sim_hvx = sim_args[i : i + 3]
i = sim_options.index("hvx_length") + len("hvx_length") + 1
sim_hvx = sim_options[i : i + 3]
if sim_hvx != str(codegen_hvx):
print(
"WARNING: sim hvx {} and codegen hvx {} mismatch!".format(
sim_hvx, codegen_hvx
)
)
msg = "sim hvx {} and codegen hvx {} mismatch!".format(sim_hvx, codegen_hvx)
# Set the stacklevel to the tvm.target.hexagon() call.
warnings.warn(msg, stacklevel=4)
elif codegen_hvx != 0:
# If --hvx_length was not given, add it if HVX is enabled
sim_args = sim_args + " " if isinstance(sim_args, str) else ""
sim_args += "--hvx_length " + str(codegen_hvx)
return sim_args or ""
sim_options = sim_options + " " if isinstance(sim_options, str) else ""
sim_options += "--hvx_length " + str(codegen_hvx)
return sim_options or ""

if not sim_args:
return cpu_ver + " " + validate_hvx_length(hvx, sim_args)
hvx = config["hvx"]
sim_options = config["sim_options"]
if not sim_options:
return cpu_ver + " " + validate_hvx_length(hvx, sim_options)

sim_cpu = cpu_ver + " "

# Add user defined args
if isinstance(sim_args, list):
sim_args = " ".join(sim_args)
if isinstance(sim_options, list):
sim_options = " ".join(sim_options)

# Check for supplied sim cpu version
if "v6" in sim_args:
if "v6" in sim_options:
sim_cpu = ""

# Regex match for allowed cpus
Expand All @@ -494,13 +533,13 @@ def validate_hvx_length(codegen_hvx, sim_args):
+ r"(?P<base_version>v6[25678])(?P<sub_version>[a-z])?"
+ r"(?P<l2_size>_[0-9]+)?(?P<rev>_rev[0-9])?\s?(?P<post>--.*)?"
)
m = re.match(valid_cpu_str_regex, sim_args.lower())
m = re.match(valid_cpu_str_regex, sim_options.lower())
if not m:
raise ValueError('Invalid simulator argument string "{}"'.format(sim_args))
raise ValueError('Invalid simulator argument string "{}"'.format(sim_options))

# Parse options into correct order
cpu_attr = {x: str(m.groupdict()[x] or "") for x in m.groupdict()}
sim_args = (
sim_options = (
cpu_attr["base_version"]
+ cpu_attr["sub_version"]
+ cpu_attr["l2_size"]
Expand All @@ -510,23 +549,27 @@ def validate_hvx_length(codegen_hvx, sim_args):
+ cpu_attr["post"]
)

return sim_cpu + " " + validate_hvx_length(hvx, sim_args)
return sim_cpu + " " + validate_hvx_length(hvx, sim_options)

# LLVM options string
def create_llvm_options(cpu_ver, config): # pylint: disable=unused-argument
""" Create LLVM options string. """

llvm_options = config["llvm_options"]

# LLVM string
def create_llvm(llvm_args):
# TVM's option parser doesn't allow '=' in values, but '=' can
# appear in LLVM flags. Replace it with '@', since it's unlikely
# that '@' will be used in another context.
if llvm_args is None or len(llvm_args.replace(" ", "")) == 0:
if llvm_options is None or len(llvm_options.strip()) == 0:
return ""
args = [s.replace("=", "@") for s in llvm_args.split()]
args = [s.replace("=", "@") for s in llvm_options.split()]
return "--llvm-options=" + ",".join(args)

# Sim args
os.environ["HEXAGON_SIM_ARGS"] = create_sim(cpu_ver, sim_args)
os.environ["HEXAGON_SIM_ARGS"] = create_sim_options(cpu_ver, config)

target_str = create_target(cpu_ver)
llvm_str = create_llvm(llvm_args)
target_str = create_llvm_target(cpu_ver, config)
llvm_str = create_llvm_options(cpu_ver, config)
args_list = target_str.split() + llvm_str.split()

return Target(" ".join(["hexagon"] + args_list))
Expand Down