Hoist vector slices using rewrite rules #7243

abadams · 2022-12-14T22:54:47Z

This lets us add associative variants more easily, which are helpful in the work on staging strided loads.

rootjalex · 2022-12-15T00:18:55Z

correctness_intrinsics is giving me an infinite loop:

    frame #146848: 0x00000001087776c0 libHalide.dylib`Halide::Internal::(anonymous namespace)::deinterleave(Halide::Expr, int, int, int, Halide::Internal::Scope<void> const&) + 364
    frame #146849: 0x0000000108777b8c libHalide.dylib`Halide::Internal::extract_lane(Halide::Expr const&, int) + 92
    frame #146850: 0x0000000108c6397c libHalide.dylib`Halide::Internal::Simplify::visit(Halide::Internal::Shuffle const*, Halide::Internal::Simplify::ExprInfo*) + 1340
    frame #146851: 0x0000000108c1cfb8 libHalide.dylib`Halide::Internal::Simplify::visit(Halide::Internal::Mul const*, Halide::Internal::Simplify::ExprInfo*) + 9128
    frame #146852: 0x0000000108b29694 libHalide.dylib`Halide::Expr Halide::Internal::Simplify::simplify_let<Halide::Internal::Let, Halide::Expr>(Halide::Internal::Let const*, Halide::Internal::Simplify::ExprInfo*) + 9456
    frame #146853: 0x0000000108a8f500 libHalide.dylib`Halide::Internal::simplify(Halide::Expr const&, bool, Halide::Internal::Scope<Halide::Internal::Interval> const&, Halide::Internal::Scope<Halide::Internal::ModulusRemainder> const&) + 56
    frame #146854: 0x00000001087776c0 libHalide.dylib`Halide::Internal::(anonymous namespace)::deinterleave(Halide::Expr, int, int, int, Halide::Internal::Scope<void> const&) + 364
<repeated>

src/IRMatch.h

src/Simplify_Add.cpp

src/IRMatch.h

The Shuffle visitor wants to sink them

abadams · 2022-12-15T16:09:55Z

Turns out the shuffle visitor wants to sink extract_element calls, and extract_element counts as a degenerate slice_vector, so it was also being hoisted. I restricted my rewrite rules to non-degenerate slice_vectors

rootjalex

Lgtm pending green

rootjalex · 2022-12-15T18:08:53Z

Turns out the shuffle visitor wants to sink extract_element calls, and extract_element counts as a degenerate slice_vector, so it was also being hoisted. I restricted my rewrite rules to non-degenerate slice_vectors

Might be worth adding a comment about this on this above the rules.

src/Simplify_Add.cpp

steven-johnson · 2023-01-09T18:06:32Z

Any update on this PR?

abadams · 2023-01-09T18:18:14Z

If the bots go green it's good to merge. Otherwise I'll need to make time to debug after the siggraph deadline.

steven-johnson · 2023-01-09T18:36:23Z

How risky do you think this is? Does it need a preflight test inside google?

abadams · 2023-01-09T18:52:56Z

It's probably worth a preflight test.

steven-johnson · 2023-01-10T22:04:50Z

At least one failure inside google:

Internal Error at third_party/halide/halide/src/IR.cpp:42 triggered by user code at : Condition failed: a.type() == b.type(): Add of mismatched types
*** SIGABRT received by PID 424 (TID 424) on cpu 30 from PID 424; stack trace: ***
PC: @     0x7fa7c0bf1347  (unknown)  gsignal
    @     0x7fa6bfc876dd        256  base/process_state.cc:1202 FailureSignalHandler()
    @     0x7fa801f071c0  (unknown)  (unknown)
    @     0x7fa77537b003        160  third_party/halide/halide/src/Error.cpp:221 Halide::Internal::ErrorReport::~ErrorReport()
    @     0x7fa775567736        160  third_party/halide/halide/src/IR.cpp:42 Halide::Internal::Add::make()
    @     0x7fa77588f0ea        192  third_party/halide/halide/src/IRMatch.h:736 Halide::Internal::IRMatcher::Rewriter<>::build_replacement<>()
    @     0x7fa775860dd9        224  third_party/halide/halide/src/IRMatch.h:2916 Halide::Internal::Simplify::visit()::$_0::operator()()
    @     0x7fa775857cfa        256  third_party/halide/halide/src/Simplify_Add.cpp:40 Halide::Internal::Simplify::visit()
    @     0x7fa775838a2f        128  third_party/halide/halide/src/IRVisitor.h:184 Halide::Internal::VariadicVisitor<>::dispatch_expr<>()
    @     0x7fa775b875f1        224  third_party/halide/halide/src/IRVisitor.h:341 Halide::Internal::Simplify::visit()
    @     0x7fa775838a51        128  third_party/halide/halide/src/IRVisitor.h:186 Halide::Internal::VariadicVisitor<>::dispatch_expr<>()
    @     0x7fa775af8143        288  third_party/halide/halide/src/IRVisitor.h:341 Halide::Internal::Simplify::visit()
    @     0x7fa775838a95        128  third_party/halide/halide/src/IRVisitor.h:190 Halide::Internal::VariadicVisitor<>::dispatch_expr<>()
    @     0x7fa77585771f        256  third_party/halide/halide/src/IRVisitor.h:341 Halide::Internal::Simplify::visit()
    @     0x7fa775838a2f        128  third_party/halide/halide/src/IRVisitor.h:184 Halide::Internal::VariadicVisitor<>::dispatch_expr<>()

EDIT: the mismatched types are float32x6 and float32x8

rootjalex · 2023-01-11T18:57:36Z

Seems like all of the rules need a check that the lanes of x and y match

abadams · 2023-01-11T19:49:36Z

Good point, that's probably the issue.

steven-johnson · 2023-01-20T17:46:36Z

(re-requested review so this doesn't accidentally get merged yet)

…ice_refactor

rootjalex · 2023-01-20T18:34:18Z

@steven-johnson Could you check inside Google again, when you have the chance? I think the bug you're seeing should be fixed now.

steven-johnson · 2023-01-20T18:45:41Z

will do

steven-johnson · 2023-01-20T18:53:59Z

Running check now. Does this PR need a test?

rootjalex · 2023-01-20T18:57:46Z

Running check now. Does this PR need a test?

Good point. I'll try to find time this afternoon to add a test for the improved hoisting and another one for the bug you found.

abadams · 2023-01-20T19:07:55Z

Thanks for taking this over, AJ.

rootjalex · 2023-01-20T23:25:23Z

Failures seem unrelated? One build failure and a mullapudi2016 issue (#7292). @steven-johnson is performance_inner_loop_parallel sometimes flaky? Or is that something I should investigate?

steven-johnson · 2023-01-21T18:16:11Z

Both of those are known to be flaky, unfortunately We need to track them down but they aren't related.

steven-johnson · 2023-01-21T18:32:18Z

Looks clean inside Google.

* Hoist slices using rewrite rules This lets us add associative variants more easily, which are helpful in the work on staging strided loads. * Don't hoist extract_element shuffles The Shuffle visitor wants to sink them * Add some static asserts * Add explanatory comment on shuffle hoisting * Fix comment * add lanes predicate to slice hoisting * add vector slice hoisting test cases Co-authored-by: Steven Johnson <srj@google.com> Co-authored-by: Alexander <ajroot@stanford.edu>

Hoist slices using rewrite rules

1a05a17

This lets us add associative variants more easily, which are helpful in the work on staging strided loads.

abadams requested a review from rootjalex December 14, 2022 22:55

rootjalex reviewed Dec 15, 2022

View reviewed changes

src/IRMatch.h Show resolved Hide resolved

rootjalex reviewed Dec 15, 2022

View reviewed changes

src/Simplify_Add.cpp Outdated Show resolved Hide resolved

rootjalex reviewed Dec 15, 2022

View reviewed changes

src/IRMatch.h Show resolved Hide resolved

abadams added 2 commits December 15, 2022 06:56

Don't hoist extract_element shuffles

1788b69

The Shuffle visitor wants to sink them

Add some static asserts

cd5bb4a

rootjalex approved these changes Dec 15, 2022

View reviewed changes

Add explanatory comment on shuffle hoisting

5f58b95

rootjalex reviewed Dec 15, 2022

View reviewed changes

src/Simplify_Add.cpp Outdated Show resolved Hide resolved

abadams and others added 2 commits December 15, 2022 13:17

Fix comment

582b98f

Merge branch 'main' into abadams/hoist_slice_refactor

54610ee

Merge branch 'main' into abadams/hoist_slice_refactor

11ca4b5

steven-johnson requested a review from rootjalex January 20, 2023 17:46

rootjalex added 2 commits January 20, 2023 10:05

Merge branch 'main' of github.com:halide/Halide into abadams/hoist_sl…

2639f47

…ice_refactor

add lanes predicate to slice hoisting

6698e0b

add vector slice hoisting test cases

a25be53

rootjalex merged commit 810bd0b into main Jan 21, 2023

rootjalex deleted the abadams/hoist_slice_refactor branch January 21, 2023 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hoist vector slices using rewrite rules #7243

Hoist vector slices using rewrite rules #7243

abadams commented Dec 14, 2022

rootjalex commented Dec 15, 2022

abadams commented Dec 15, 2022

rootjalex left a comment

rootjalex commented Dec 15, 2022

steven-johnson commented Jan 9, 2023

abadams commented Jan 9, 2023

steven-johnson commented Jan 9, 2023

abadams commented Jan 9, 2023

steven-johnson commented Jan 10, 2023 •

edited

Loading

rootjalex commented Jan 11, 2023

abadams commented Jan 11, 2023

steven-johnson commented Jan 20, 2023

rootjalex commented Jan 20, 2023

steven-johnson commented Jan 20, 2023

steven-johnson commented Jan 20, 2023

rootjalex commented Jan 20, 2023

abadams commented Jan 20, 2023

rootjalex commented Jan 20, 2023 •

edited

Loading

steven-johnson commented Jan 21, 2023

steven-johnson commented Jan 21, 2023

Hoist vector slices using rewrite rules #7243

Hoist vector slices using rewrite rules #7243

Conversation

abadams commented Dec 14, 2022

rootjalex commented Dec 15, 2022

abadams commented Dec 15, 2022

rootjalex left a comment

Choose a reason for hiding this comment

rootjalex commented Dec 15, 2022

steven-johnson commented Jan 9, 2023

abadams commented Jan 9, 2023

steven-johnson commented Jan 9, 2023

abadams commented Jan 9, 2023

steven-johnson commented Jan 10, 2023 • edited Loading

rootjalex commented Jan 11, 2023

abadams commented Jan 11, 2023

steven-johnson commented Jan 20, 2023

rootjalex commented Jan 20, 2023

steven-johnson commented Jan 20, 2023

steven-johnson commented Jan 20, 2023

rootjalex commented Jan 20, 2023

abadams commented Jan 20, 2023

rootjalex commented Jan 20, 2023 • edited Loading

steven-johnson commented Jan 21, 2023

steven-johnson commented Jan 21, 2023

steven-johnson commented Jan 10, 2023 •

edited

Loading

rootjalex commented Jan 20, 2023 •

edited

Loading