-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
"""Returns a Hexagon target. | ||
|
||
Parameters | ||
---------- | ||
cpu_ver : str | ||
cpu_ver : str (default: "v66") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 | ||
|
@@ -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"] | ||
|
@@ -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)) | ||
|
There was a problem hiding this comment.
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
andllvm_args
later.There was a problem hiding this comment.
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 byargs
.There was a problem hiding this comment.
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
andcpu_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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed
args
toconfig
, hopefully it will better reflect the purpose of the variable.