Skip to content

Add one-sided widening intrinsics. #6967

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

Merged
rootjalex merged 10 commits into from
Sep 13, 2022
Merged

Add one-sided widening intrinsics. #6967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
420b583
implement widen_right_ ops
Aug 22, 2022
70c8cb8
clang-format
Aug 22, 2022
81c650b
update HVX patterns with one-sided widening intrinsics
Aug 23, 2022
95f90bc
don't turn VectorReduce nodes into widen_right_adds
Aug 23, 2022
c344e82
remove unused HVX pattern flags
Aug 23, 2022
3bcbfe1
clang tidy
Aug 23, 2022
938bb04
stupid bug fix
Aug 24, 2022
8e3a6cf
lower widen_right_sub/widen_right_add in HexagonOptimize
Aug 26, 2022
2d9ebe0
recursively mutate widen_right variants
Aug 31, 2022
6c81f33
Merge branch 'main' into rootjalex/extend-intrinsics
steven-johnson Sep 9, 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
1 change: 1 addition & 0 deletions src/CodeGen_ARM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,6 +1331,7 @@ void CodeGen_ARM::codegen_vector_reduce(const VectorReduce *op, const Expr &init
if (narrow.defined()) {
if (init.defined() && target.bits == 32) {
// On 32-bit, we have an intrinsic for widening add-accumulate.
// TODO: this could be written as a pattern with widen_right_add (#6951).
intrin = "pairwise_widening_add_accumulate";
intrin_args = {accumulator, narrow};
accumulator = Expr();
Expand Down
208 changes: 202 additions & 6 deletions src/FindIntrinsics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ bool is_safe_for_add(const Expr &e, int max_depth) {
} else if (cast->type.bits() == cast->value.type().bits()) {
return is_safe_for_add(cast->value, max_depth);
}
} else if (Call::as_intrinsic(e, {Call::widening_add, Call::widening_sub})) {
} else if (Call::as_intrinsic(e, {Call::widening_add, Call::widening_sub, Call::widen_right_add, Call::widen_right_sub})) {
return true;
}
return false;
Expand Down Expand Up @@ -131,20 +131,47 @@ Expr to_rounding_shift(const Call *c) {
}
Expr round;
if (c->is_intrinsic(Call::shift_right)) {
round = simplify((make_one(round_type) << max(cast(b.type().with_bits(round_type.bits()), b), 0)) / 2);
round = (make_one(round_type) << max(cast(b.type().with_bits(round_type.bits()), b), 0)) / 2;
} else {
round = simplify((make_one(round_type) >> min(cast(b.type().with_bits(round_type.bits()), b), 0)) / 2);
round = (make_one(round_type) >> min(cast(b.type().with_bits(round_type.bits()), b), 0)) / 2;
}
// Input expressions are simplified before running find_intrinsics, but b
// has been lifted here so we need to lower_intrinsics before simplifying
// and re-lifting. Should we move this code into the FindIntrinsics class
// to make it easier to lift round?
round = lower_intrinsics(round);
round = simplify(round);
round = find_intrinsics(round);

// We can always handle widening adds.
if (const Call *add = Call::as_intrinsic(a, {Call::widening_add})) {
if (can_prove(lower_intrinsics(add->args[0]) == round)) {
if (can_prove(lower_intrinsics(add->args[0] == round))) {
return rounding_shift(cast(add->type, add->args[1]), b);
} else if (can_prove(lower_intrinsics(add->args[1]) == round)) {
} else if (can_prove(lower_intrinsics(add->args[1] == round))) {
return rounding_shift(cast(add->type, add->args[0]), b);
}
}

if (const Call *add = Call::as_intrinsic(a, {Call::widen_right_add})) {
if (can_prove(lower_intrinsics(add->args[1] == round))) {
return rounding_shift(cast(add->type, add->args[0]), b);
}
}
// Also need to handle the annoying case of a reinterpret wrapping a widen_right_add
// TODO: this pattern makes me want to change the semantics of this op.
if (const Reinterpret *reinterp = a.as<Reinterpret>()) {
if (reinterp->type.bits() == reinterp->value.type().bits()) {
if (const Call *add = Call::as_intrinsic(reinterp->value, {Call::widen_right_add})) {
if (can_prove(lower_intrinsics(add->args[1] == round))) {
// We expect the first operand to be a reinterpet.
const Reinterpret *reinterp_a = add->args[0].as<Reinterpret>();
internal_assert(reinterp_a) << "Failed: " << add->args[0] << "\n";
return rounding_shift(reinterp_a->value, b);
}
}
}
}

// If it wasn't a widening or saturating add, we might still
// be able to safely accept the rounding.
Expr a_less_round = find_and_subtract(a, round);
Expand Down Expand Up @@ -199,6 +226,53 @@ class FindIntrinsics : public IRMutator {
}
}

if (op->type.is_int_or_uint() && op->type.bits() > 8) {
// Look for widen_right_add intrinsics.
// Yes we do an duplicate code, but we want to check the op->type.code() first,
// and the opposite as well.
for (halide_type_code_t code : {op->type.code(), halide_type_uint, halide_type_int}) {
Type narrow = op->type.narrow().with_code(code);
// Pulling casts out of VectorReduce nodes breaks too much codegen, skip for now.
Expr narrow_a = (a.node_type() == IRNodeType::VectorReduce) ? Expr() : lossless_cast(narrow, a);
Expr narrow_b = (b.node_type() == IRNodeType::VectorReduce) ? Expr() : lossless_cast(narrow, b);

// This case should have been handled by the above check for widening_add.
internal_assert(!(narrow_a.defined() && narrow_b.defined()))
<< "find_intrinsics failed to find a widening_add: " << a << " + " << b << "\n";

if (narrow_a.defined()) {
Expr result;
if (b.type().code() != narrow_a.type().code()) {
// Need to do a safe reinterpret.
Type t = b.type().with_code(code);
result = widen_right_add(reinterpret(t, b), narrow_a);
internal_assert(result.type() != op->type);
result = reinterpret(op->type, result);
} else {
result = widen_right_add(b, narrow_a);
}
internal_assert(result.type() == op->type);
return result;
} else if (narrow_b.defined()) {
Expr result;
if (a.type().code() != narrow_b.type().code()) {
// Need to do a safe reinterpret.
Type t = a.type().with_code(code);
result = widen_right_add(reinterpret(t, a), narrow_b);
internal_assert(result.type() != op->type);
result = reinterpret(op->type, result);
} else {
result = widen_right_add(a, narrow_b);
}
internal_assert(result.type() == op->type);
return mutate(result);
}
}
}

// TODO: there can be widen_right_add + widen_right_add simplification rules.
// i.e. widen_right_add(a, b) + widen_right_add(c, d) = (a + c) + widening_add(b, d)

if (a.same_as(op->a) && b.same_as(op->b)) {
return op;
} else {
Expand Down Expand Up @@ -240,6 +314,32 @@ class FindIntrinsics : public IRMutator {
return Add::make(a, negative_b);
}

// Run after the lossless_negate check, because we want that to turn into an widen_right_add if relevant.
if (op->type.is_int_or_uint() && op->type.bits() > 8) {
// Look for widen_right_sub intrinsics.
// Yes we do an duplicate code, but we want to check the op->type.code() first,
// and the opposite as well.
for (halide_type_code_t code : {op->type.code(), halide_type_uint, halide_type_int}) {
Type narrow = op->type.narrow().with_code(code);
Expr narrow_b = lossless_cast(narrow, b);

if (narrow_b.defined()) {
Expr result;
if (a.type().code() != narrow_b.type().code()) {
// Need to do a safe reinterpret.
Type t = a.type().with_code(code);
result = widen_right_sub(reinterpret(t, a), narrow_b);
internal_assert(result.type() != op->type);
result = reinterpret(op->type, result);
} else {
result = widen_right_sub(a, narrow_b);
}
internal_assert(result.type() == op->type);
return mutate(result);
}
}
}

if (a.same_as(op->a) && b.same_as(op->b)) {
return op;
} else {
Expand Down Expand Up @@ -292,6 +392,49 @@ class FindIntrinsics : public IRMutator {
return mutate(result);
}

if (op->type.is_int_or_uint() && op->type.bits() > 8) {
// Look for widen_right_mul intrinsics.
// Yes we do an duplicate code, but we want to check the op->type.code() first,
// and the opposite as well.
for (halide_type_code_t code : {op->type.code(), halide_type_uint, halide_type_int}) {
Type narrow = op->type.narrow().with_code(code);
Expr narrow_a = lossless_cast(narrow, a);
Expr narrow_b = lossless_cast(narrow, b);

// This case should have been handled by the above check for widening_mul.
internal_assert(!(narrow_a.defined() && narrow_b.defined()))
<< "find_intrinsics failed to find a widening_mul: " << a << " + " << b << "\n";

if (narrow_a.defined()) {
Expr result;
if (b.type().code() != narrow_a.type().code()) {
// Need to do a safe reinterpret.
Type t = b.type().with_code(code);
result = widen_right_mul(reinterpret(t, b), narrow_a);

@vksnk vksnk Sep 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(This is a follow-up to another comment from Steven regarding failures in C++ backend).

I am a bit confused by reinterpret here, do I understand it correctly that it might turn wild_i32x * u32(wild_u16x) into widen_right_mul(wild_u32x, wild_u16x)?

The problem we are seeing is in the Xtensa backend (not exactly a C++ backend, but it's derived from it; also, lives in a separate branch, so it's a bit harder to keep track of), we currently don't handle this intrinsic, so at the code generation stage we will call lower_intrinsic once we encounter widen_right_mul. As a result, the following will happen:

  1. we start with the expession like wild_i32x * widen(wild_u16x)
  2. it gets transformed into widen_right_mul(wild_u32x, wild_u16x)
  3. it gets lowered back to wild_u32x * widen(wild_u32x) [notice that left operand became unsingned]

If that's correct then the input of the 1) is not equiualent to the output of the 3), which seems a bit problematic? Is this transformation correct from numerical point of view (I guess depends on the actual implementation)? The specific problem we see in the Xtensa backend, is that Xtensa doesn't seem to have an intrinsic for multiplication of two wild_u32x vectors, but does have intrinsics for wild_i32x * wild_i32x and wild_i32x * wild_u32x (I know it's a bit weird and I can try to find out more details about it, but it may take some time).

@rootjalex rootjalex Sep 2, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Numerically, the expressions are still equivalent, integer multiplication is the same for signed or unsigned arguments.

I see your issue though, and I see two possible solutions:

  1. in lowering intrinsics, specifically look for the pattern reinterpret(widen_right_op(reinterpret(a), b)), and lower it without the reinterprets (this seems messy)

  2. Xtensa should specifically pattern match widen_right_mul(reinterpret(i32), u16) and use the wild_i32 * wild_u32 op that you mentioned

I believe that the letter is better, what do you think? Feasibly both could be implemented.

@rootjalex rootjalex Sep 2, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess we could also change the design of the intrinsics to be
a op cast(a.type(), b)
I think there was some reason that we chose not to do that initially. @abadams do you remember why?

@vksnk vksnk Sep 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, 1) would be better in my opinion, but seems to be difficult to implement due to the outer reinterpret(), so probably not worth the effort or complexity.

I certainly can do 2), this should be pretty straigtforward. I was concerned that expressions are not equiualent after find_intrinsics -> lower_intrinsic, but it sounds it should be good numeric-wise.

I think, if this is the only issue we see in Google testing then it should be fine to merge in and I can address the issue before updating Halide in Google.

@steven-johnson steven-johnson Sep 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd prefer we address the Xtensa issue first, so that we can complete a test of this change in Google before landing. (Currently there are a lot of false-positive failures in the test due to this.)

@vksnk vksnk Sep 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's cool with me, I'll look into it.

@rootjalex rootjalex Sep 3, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@vksnk I realize now that I should have questioned deeper on the Xtensa multiplication intrinsics - because multiplication is not parameterized by sign, shouldn’t the intrinsic that you mentioned being used for i32 x i32 multiplication be used for any 32-bit integer multiplication?

That being said, I’m a little unsure what the i32 x u32 multiplication is used for. Is that a widening multiply by chance?

@rootjalex rootjalex Sep 3, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

And to address your concern about find_intrinsics -> lower_intrinsics not matching perfectly - unfortunately, that is already the case for most (possibly all?) of the intrinsics, though I believe these are the only intrinsics that will add reinterprets

internal_assert(result.type() != op->type);
result = reinterpret(op->type, result);
} else {
result = widen_right_mul(b, narrow_a);
}
internal_assert(result.type() == op->type);
return result;
} else if (narrow_b.defined()) {
Expr result;
if (a.type().code() != narrow_b.type().code()) {
// Need to do a safe reinterpret.
Type t = a.type().with_code(code);
result = widen_right_mul(reinterpret(t, a), narrow_b);
internal_assert(result.type() != op->type);
result = reinterpret(op->type, result);
} else {
result = widen_right_mul(a, narrow_b);
}
internal_assert(result.type() == op->type);
return mutate(result);
}
}
}

if (a.same_as(op->a) && b.same_as(op->b)) {
return op;
} else {
Expand Down Expand Up @@ -594,6 +737,37 @@ class FindIntrinsics : public IRMutator {
const auto is_x_wider_opposite_int = (op->type.is_int() && is_uint(x, 2 * bits)) || (op->type.is_uint() && is_int(x, 2 * bits));

if (
// Simplify extending patterns.
// (x + widen(y)) + widen(z) = x + widening_add(y, z).
rewrite(widen_right_add(widen_right_add(x, y), z),
x + widening_add(y, z),
// We only care about integers, this should be trivially true.
is_x_same_int_or_uint) ||

// (x - widen(y)) - widen(z) = x - widening_add(y, z).
rewrite(widen_right_sub(widen_right_sub(x, y), z),
x - widening_add(y, z),
// We only care about integers, this should be trivially true.
is_x_same_int_or_uint) ||

// (x + widen(y)) - widen(z) = x + cast(t, widening_sub(y, z))
// cast (reinterpret) is needed only for uints.
rewrite(widen_right_sub(widen_right_add(x, y), z),
x + widening_sub(y, z),
is_x_same_int) ||
rewrite(widen_right_sub(widen_right_add(x, y), z),
x + cast(op->type, widening_sub(y, z)),
is_x_same_uint) ||

// (x - widen(y)) + widen(z) = x + cast(t, widening_sub(z, y))
// cast (reinterpret) is needed only for uints.
rewrite(widen_right_add(widen_right_sub(x, y), z),
x + widening_sub(z, y),
is_x_same_int) ||
rewrite(widen_right_add(widen_right_sub(x, y), z),
x + cast(op->type, widening_sub(z, y)),
is_x_same_uint) ||

// Saturating patterns.
rewrite(saturating_cast(op->type, widening_add(x, y)),
saturating_add(x, y),
Expand Down Expand Up @@ -679,6 +853,7 @@ class FindIntrinsics : public IRMutator {
}
}
}
// TODO: do we want versions of widen_right_add here?

if (op->is_intrinsic(Call::shift_right) || op->is_intrinsic(Call::shift_left)) {
// Try to turn this into a widening shift.
Expand Down Expand Up @@ -885,6 +1060,18 @@ Expr find_intrinsics(const Expr &e) {
return expr;
}

Expr lower_widen_right_add(const Expr &a, const Expr &b) {
return a + widen(b);
}

Expr lower_widen_right_mul(const Expr &a, const Expr &b) {
return a * widen(b);
}

Expr lower_widen_right_sub(const Expr &a, const Expr &b) {
return a - widen(b);
}

Expr lower_widening_add(const Expr &a, const Expr &b) {
return widen(a) + widen(b);
}
Expand Down Expand Up @@ -1100,7 +1287,16 @@ Expr lower_rounding_mul_shift_right(const Expr &a, const Expr &b, const Expr &q)
}

Expr lower_intrinsic(const Call *op) {
if (op->is_intrinsic(Call::widening_add)) {
if (op->is_intrinsic(Call::widen_right_add)) {
internal_assert(op->args.size() == 2);
return lower_widen_right_add(op->args[0], op->args[1]);
} else if (op->is_intrinsic(Call::widen_right_mul)) {
internal_assert(op->args.size() == 2);
return lower_widen_right_mul(op->args[0], op->args[1]);
} else if (op->is_intrinsic(Call::widen_right_sub)) {
internal_assert(op->args.size() == 2);
return lower_widen_right_sub(op->args[0], op->args[1]);
} else if (op->is_intrinsic(Call::widening_add)) {
internal_assert(op->args.size() == 2);
return lower_widening_add(op->args[0], op->args[1]);
} else if (op->is_intrinsic(Call::widening_mul)) {
Expand Down
3 changes: 3 additions & 0 deletions src/FindIntrinsics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ namespace Halide {
namespace Internal {

/** Implement intrinsics with non-intrinsic using equivalents. */
Expr lower_widen_right_add(const Expr &a, const Expr &b);
Expr lower_widen_right_mul(const Expr &a, const Expr &b);
Expr lower_widen_right_sub(const Expr &a, const Expr &b);
Expr lower_widening_add(const Expr &a, const Expr &b);
Expr lower_widening_mul(const Expr &a, const Expr &b);
Expr lower_widening_sub(const Expr &a, const Expr &b);
Expand Down
Loading