Handle many more intrinsics in Bounds.cpp #7823

steven-johnson · 2023-08-29T23:43:45Z

This addresses many (but not all) of the signed integer overflow issues we're seeing in Google due to #7814 -- a lot of the issues seems to be in code that uses intrinsics that had no handling in value bounds checking, so the bounds were naively large and overflowed.

Most of the intrinsics from FindIntrinsics.h weren't handled; now they all are (most by lowering to other IR, though the halving_add variants were modeled directly because the bitwise ops don't mesh well)
strict_float() is just a pass-through
round() is a best guess (basically, if bounds exist, expand by one as a worst-case)

There are definitely others we should handle here... trunc/floor/ceil probably?

This addresses many (but not all) of the `signed integer overflow` issues we're seeing in Google due to #7814 -- a lot of the issues seems to be in code that uses intrinsics that had no handling in value bounds checking, so the bounds were naively large and overflowed. - Most of the intrinsics from FindIntrinsics.h weren't handled; now they all are (most by lowering to other IR, though the halving_add variants were modeled directly because the bitwise ops don't mesh well) - strict_float() is just a pass-through - round() is a best guess (basically, if bounds exist, expand by one as a worst-case) There are definitely others we should handle here... trunc/floor/ceil probably?

abadams · 2023-08-30T00:02:51Z

src/Bounds.cpp

+        } else if (op->is_intrinsic(Call::strict_float)) {
+            internal_assert(op->args.size() == 1);
+            interval = arg_bounds.get(0);
+        } else if (op->is_intrinsic(Call::round)) {


Round was already handled below more tightly than this (you can just round the min and max). strict_float should probably be handled in the same place. i.e. we should probably preserve the strict_float wrapper around the min and max so that floating point optimizations don't happen to the min and max in a way that makes them no longer contain the original expression.

Ah, I missed the Call::round handling below -- I think the case I saw didn't pass the (interval = arg_bounds.get(0)).is_bounded() test and that's why I thought it was missing.

abadams · 2023-08-30T00:38:04Z

src/Bounds.cpp

@@ -1468,6 +1480,7 @@ class Bounds : public IRVisitor {
            }
        } else if (op->args.size() == 1 &&
                   (op->is_intrinsic(Call::round) ||
+                    op->is_intrinsic(Call::strict_float) ||


There's going to be a merge conflict here because Call::saturating_cast is in the same category. Probably should add it in this PR in case the other one doesn't go in and we revert the u32 -> i32 cast change.

I take it back! saturating_cast doesn't belong here.

abadams · 2023-08-30T01:25:48Z

src/Bounds.cpp

+        } else if (op->is_intrinsic(Call::halving_add)) {
+            // lower_halving_add() uses bitwise tricks that are hard to reason
+            // about; let's do this instead:
+            Expr e = narrow((widen(op->args[0]) + widen(op->args[1])) / 2);


Looking at the bot failure, I suspect this is trying to widen a 64-bit input

Hm, well, ok, but this is literally the fallback implementation for it

(I don't know how to make the bitwise op handling robust enough to handle this correctly)

I guess the right way to handle this is to special-case 64-bit and use the min/max possible

I have a branch that (among other things) tacked on bounds inference for many of these intrinsics - I did have to special-case any intrinsic that semantically widens if the arguments are 64 bit, and there were a few that would produce a double-widening so had to be even further special-cases (I think rounding_mul_shift_right lowers to a widening mul followed by a rounding shift right that lowers to a widening add or something like that, so it would double-widen

I have a branch

please share! These changes make for much better bounds inference in some cases (esp pipelines with fixed-point math); if your fixes are better than these we should take yours.

I need to severely clean it up - that's part of why I never opened a PR. I can try to clean it up and share, might take me a few days unfortunately - I am about to be traveling for a funding thing.

Even if it's ugly, feel free to put it somewhere I can look at it.

This is the change in Bounds.cpp (note the TODOs I have there are out-of-date): https://github.com/halide/Halide/blob/7a497fd4369f278c16abb9790beabf40514ae22f/src/Bounds.cpp#L1522-#L1546
Here is the corresponding lowering code:
https://github.com/halide/Halide/blob/7a497fd4369f278c16abb9790beabf40514ae22f/src/FindIntrinsics.cpp#L1793-#L1908

rootjalex · 2023-08-31T18:43:04Z

src/Bounds.cpp

-            // about; let's do this instead:
-            if (op->type.bits() == 64) {
-                bounds_of_type(t);
+        } else if (op->is_intrinsic(Call::widen_right_add)) {


I don't think we need this many safety checks for the widening operations. Any Expr in a widening op needs to be able to be widened - we can't lift to widening_mul unless a user widened the inputs. We only need to be careful with operations that we can lift to without widening operations, but that the "simple" lowering pattern involves widening.

Well, yes and no -- it's true a that the Exprs in a widening op need to be widened, and well-formed code shouldn't pass us cases that don't fit; that said, we absolutely will get misuse in that way, so what should we do when that happens? IMHO we are better off checking for it an explicitly devolving to bounds-of-type, rather than risking that the bounds-calc code makes a mistake and calculates an inappropriate bound due to inadvertent overflow.

Ah, I think I misunderstood the use case. Is this for when users write code that produces a LUT index, and uses intermediate 64 bit types?

Is this for when users write code that produces a LUT index

Yes? I mean, we have no control of what the user is doing with these functions; they could pass it insane nonsense, so we need to be somewhat defensive here. We'd prefer to avoid a too-loose bounds, but we absolutely cannot risk getting too-tight bounds.

steven-johnson · 2023-09-12T22:39:12Z

@rootjalex -- obviously this didn't get landed and I was out all last week; when you get a chance, we should put our heads together to figure out how to combine our approaches.

rootjalex · 2023-09-13T20:29:29Z

@rootjalex -- obviously this didn't get landed and I was out all last week; when you get a chance, we should put our heads together to figure out how to combine our approaches.

I am absolutely swamped this week, and have a 9/19 deadline. Happy to discuss at the dev meeting (I should be able to attend this week, I hope...), or sometime after 9/19. Sorry about that

steven-johnson · 2023-09-25T23:15:36Z

Hey @rootjalex, I'm not going to have time to sync up with you on this for a bit -- you are welcome to take over this PR and combine it with your own as you see fit (or ignore it entirely if yours looks better); otherwise this will likely sit unfinished until sometime in November

steven-johnson · 2023-11-28T15:20:48Z

This has been sitting here a while. Where does it stand? Does it need more work?

abadams · 2023-11-28T15:35:40Z

I think this one is a partial fix for problems identified in #7814

steven-johnson · 2023-11-30T18:00:48Z

This PR is definitely not a complete fix, but I think it is worthy of landing as a partial fix (pending checking in Google) -- WDYT?

rootjalex · 2023-11-30T18:11:24Z

src/Bounds.cpp

@@ -41,6 +41,36 @@ using std::string;
 using std::vector;

 namespace {
+
+bool can_widen(const Expr &e) {
+    return e.type().bits() < 64;


This should probably be <= 32. I'm thinking of the 48 bit types in the xtensa backend.

rootjalex · 2023-11-30T18:13:05Z

This PR is definitely not a complete fix, but I think it is worthy of landing as a partial fix (pending checking in Google) -- WDYT?

I think that's fine. Sorry for going AWOL - I had a big deadline recently that took all of my time.

steven-johnson · 2023-11-30T18:30:47Z

Hmm, this injects a lot of signed-integer-overflow failures in google3... I'll need to do some debugging.

steven-johnson · 2023-11-30T18:50:21Z

My implementation of saturating_cast was misguided, I just reverted it entirely for now

* Handle many more intrinsics in Bounds.cpp This addresses many (but not all) of the `signed integer overflow` issues we're seeing in Google due to halide#7814 -- a lot of the issues seems to be in code that uses intrinsics that had no handling in value bounds checking, so the bounds were naively large and overflowed. - Most of the intrinsics from FindIntrinsics.h weren't handled; now they all are (most by lowering to other IR, though the halving_add variants were modeled directly because the bitwise ops don't mesh well) - strict_float() is just a pass-through - round() is a best guess (basically, if bounds exist, expand by one as a worst-case) There are definitely others we should handle here... trunc/floor/ceil probably? * Fix round() and strict_float() handling * Update Bounds.cpp * Fixes? * trigger buildbots * Revert saturating_cast handling * Update Bounds.cpp --------- Co-authored-by: Andrew Adams <andrew.b.adams@gmail.com>

steven-johnson requested a review from abadams August 29, 2023 23:43

abadams reviewed Aug 30, 2023

View reviewed changes

Fix round() and strict_float() handling

fd0b475

abadams reviewed Aug 30, 2023

View reviewed changes

steven-johnson added 3 commits August 29, 2023 18:33

Update Bounds.cpp

0d5b3e4

Merge branch 'main' into srj/intrinsics-bounds

32eb37d

Fixes?

c788294

rootjalex reviewed Aug 31, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into srj/intrinsics-bounds

afa7feb

Merge branch 'main' into srj/intrinsics-bounds

1888074

steven-johnson added 2 commits November 29, 2023 14:36

Merge branch 'main' into srj/intrinsics-bounds

7d958f6

trigger buildbots

2bcb879

rootjalex reviewed Nov 30, 2023

View reviewed changes

rootjalex approved these changes Nov 30, 2023

View reviewed changes

Revert saturating_cast handling

13745f8

Update Bounds.cpp

a42428f

steven-johnson merged commit 4fc2a7d into main Dec 1, 2023
19 checks passed

steven-johnson deleted the srj/intrinsics-bounds branch December 1, 2023 00:31

BrewTestBot mentioned this pull request Feb 2, 2024

halide 17.0.0 Homebrew/homebrew-core#161602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle many more intrinsics in Bounds.cpp #7823

Handle many more intrinsics in Bounds.cpp #7823

steven-johnson commented Aug 29, 2023

abadams Aug 30, 2023

steven-johnson Aug 30, 2023

abadams Aug 30, 2023

abadams Aug 30, 2023

abadams Aug 30, 2023

steven-johnson Aug 30, 2023

steven-johnson Aug 30, 2023

steven-johnson Aug 30, 2023

rootjalex Aug 30, 2023

steven-johnson Aug 30, 2023

rootjalex Aug 30, 2023

steven-johnson Aug 30, 2023

rootjalex Aug 30, 2023

rootjalex Aug 31, 2023

steven-johnson Aug 31, 2023

rootjalex Aug 31, 2023

steven-johnson Aug 31, 2023

steven-johnson commented Sep 12, 2023

rootjalex commented Sep 13, 2023

steven-johnson commented Sep 25, 2023

steven-johnson commented Nov 28, 2023

abadams commented Nov 28, 2023

steven-johnson commented Nov 30, 2023

rootjalex Nov 30, 2023

steven-johnson Nov 30, 2023

rootjalex commented Nov 30, 2023

steven-johnson commented Nov 30, 2023

steven-johnson commented Nov 30, 2023

Handle many more intrinsics in Bounds.cpp #7823

Handle many more intrinsics in Bounds.cpp #7823

Conversation

steven-johnson commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson commented Sep 12, 2023

rootjalex commented Sep 13, 2023

steven-johnson commented Sep 25, 2023

steven-johnson commented Nov 28, 2023

abadams commented Nov 28, 2023

steven-johnson commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootjalex commented Nov 30, 2023

steven-johnson commented Nov 30, 2023

steven-johnson commented Nov 30, 2023