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.

Sign up for GitHub

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

Jump to bottom

Add Target support for architectures with implementation specific vector size. #6786

Merged
merged 5 commits into from
May 31, 2022
Merged

Add Target support for architectures with implementation specific vector size. #6786

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d94ae0
Move vector_bits_* Target support from fixed_width_vectors branch to
May 26, 2022
062213c
Code formatting fix.
May 26, 2022
3c1a47d
More formatting.
May 26, 2022
545ca81
Fix vector_bits output formatting . Add test coverage.
May 26, 2022
022fb51
Formatting.
May 27, 2022
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
42 changes: 40 additions & 2 deletions src/Target.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ Target calculate_host_target() {

bool use_64_bits = (sizeof(size_t) == 8);
int bits = use_64_bits ? 64 : 32;
int vector_bits = 0;
Target::Processor processor = Target::Processor::ProcessorGeneric;
std::vector<Target::Feature> initial_features;

Expand Down Expand Up @@ -296,7 +297,7 @@ Target calculate_host_target() {
#endif
#endif

return {os, arch, bits, processor, initial_features};
return {os, arch, bits, processor, initial_features, vector_bits};
}

bool is_using_hexagon(const Target &t) {
Expand Down Expand Up @@ -539,6 +540,18 @@ bool lookup_feature(const std::string &tok, Target::Feature &result) {
return false;
}

int parse_vector_bits(const std::string &tok) {
if (tok.find("vector_bits_") == 0) {
std::string num = tok.substr(sizeof("vector_bits_") - 1, std::string::npos);
size_t end_index;
int parsed = std::stoi(num, &end_index);
if (end_index == num.size()) {
return parsed;
}
}
return -1;
}

} // End anonymous namespace

Target get_target_from_environment() {
Expand Down Expand Up @@ -601,6 +614,7 @@ bool merge_string(Target &t, const std::string &target) {
for (size_t i = 0; i < tokens.size(); i++) {
const string &tok = tokens[i];
Target::Feature feature;
int vector_bits;

if (tok == "host") {
if (i > 0) {
Expand Down Expand Up @@ -636,6 +650,8 @@ bool merge_string(Target &t, const std::string &target) {
} else if (tok == "trace_all") {
t.set_features({Target::TraceLoads, Target::TraceStores, Target::TraceRealizations});
features_specified = true;
} else if ((vector_bits = parse_vector_bits(tok)) >= 0) {
t.vector_bits = vector_bits;
} else {
return false;
}
Expand Down Expand Up @@ -801,6 +817,10 @@ std::string Target::to_string() const {
if (has_feature(Target::TraceLoads) && has_feature(Target::TraceStores) && has_feature(Target::TraceRealizations)) {
result = Internal::replace_all(result, "trace_loads-trace_realizations-trace_stores", "trace_all");
}
if (vector_bits != 0) {
result += "vector_bits:" + std::to_string(vector_bits);
Copy link
Contributor

Choose a reason for hiding this comment

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

(1) There is lots of code that assumes that Target::to_string() is roundtribble with the Target ctor, so this really needs to emit this as -vector_bits_%d to match the parser
(2) to_string() is expected to emit a 'canonical' version of the Target string, so order matters. Currently, we emit all features in alphabetical order (because std::map). Since this is parsed as though it were a feature, IMHO it should be emitted as a feature (and thus sorted with the v's). I guess we could declare it to always come last, but it will be a little weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

The colon is left over from a previous syntax. I thought I had test coverage for that.

Copy link
Member Author

@zvookin zvookin May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed the formatting and added test coverage.

The idea here is that this is not a feature flag because it doesn't show up in the feature flags set. I feel like any code dealing with it needs to know that it can have an arbitrary number inside it anyway, so the fact that it canonicalizes to the end is fine, and perhaps a good signal that it is a bit different.

One issue is this doesn't play well with the can_use_target features machinery. We'll have to extend that anyway to handle an arbitrary numeric parameter. (And I really don't want to allocate a large number of feature flags to represent all the likely sizes.)

}

return result;
}

Expand Down Expand Up @@ -1061,7 +1081,15 @@ int Target::natural_vector_size(const Halide::Type &t) const {
const bool is_integer = t.is_int() || t.is_uint();
const int data_size = t.bytes();

if (arch == Target::Hexagon) {
if (arch == Target::ARM) {
if (vector_bits != 0 &&
(has_feature(Halide::Target::SVE2) ||
(t.is_float() && has_feature(Halide::Target::SVE)))) {
return vector_bits / (data_size * 8);
} else {
return 16 / data_size;
}
} else if (arch == Target::Hexagon) {
if (is_integer) {
if (has_feature(Halide::Target::HVX)) {
return 128 / data_size;
Expand Down Expand Up @@ -1103,6 +1131,13 @@ int Target::natural_vector_size(const Halide::Type &t) const {
// No vectors, sorry.
return 1;
}
} else if (arch == Target::RISCV) {
if (vector_bits != 0 &&
has_feature(Halide::Target::RVV)) {
return vector_bits / (data_size * 8);
} else {
return 1;
}
} else {
// Assume 128-bit vectors on other targets.
return 16 / data_size;
Expand Down Expand Up @@ -1310,6 +1345,9 @@ void target_test() {
}
}

internal_assert(Target().vector_bits == 0) << "Default Target vector_bits not 0.\n";
internal_assert(Target("arm-64-linux-sve2-vector_bits_512").vector_bits == 512) << "Vector bits not round tripped in Target.\n";

std::cout << "Target test passed" << std::endl;
}

Expand Down
10 changes: 8 additions & 2 deletions src/Target.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ struct Target {
/** The bit-width of the target machine. Must be 0 for unknown, or 32 or 64. */
int bits = 0;

/** The bit-width of a vector register for targets where this is configurable and
* targeting a fixed size is desired. The default of 0 indicates no assumption of
* fixed size is allowed. */
int vector_bits = 0;

/** The specific processor to be targeted, tuned for.
* Corresponds to processor_name_map in Target.cpp.
*
Expand Down Expand Up @@ -159,8 +164,9 @@ struct Target {
FeatureEnd = halide_target_feature_end
};
Target() = default;
Target(OS o, Arch a, int b, Processor pt, const std::vector<Feature> &initial_features = std::vector<Feature>())
: os(o), arch(a), bits(b), processor_tune(pt) {
Target(OS o, Arch a, int b, Processor pt, const std::vector<Feature> &initial_features = std::vector<Feature>(),
int vb = 0)
: os(o), arch(a), bits(b), vector_bits(vb), processor_tune(pt) {
for (const auto &f : initial_features) {
set_feature(f);
}
Expand Down