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.

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

Getting InternalError with a combination of compute_with, and reductions that use functions with update definitions #8149

Closed
sakehl opened this issue Mar 11, 2024 · 2 comments · Fixed by #8152
Assignees
Labels

Comments

@sakehl
Copy link
Contributor

sakehl commented Mar 11, 2024

This example program

#include "Halide.h"

using namespace Halide;

int main(int argc, char **argv){
  Func denominator_inter("denominator_inter");
  Func numerator("numerator"), denominator("denominator");
  Var i("i"), si("si"), vis("vis");

  numerator(si) = 0;
  denominator(si) = 0;
  RDom rv(0, 10, "rv");

  denominator_inter(i, vis) = 0;
  denominator_inter(0, vis) = 1 + vis;
  denominator(rv) += denominator_inter(0, rv);

  numerator(rv) += rv;
  numerator.compute_root();
  denominator.compute_root();
  denominator.update().compute_with(numerator.update(), rv);

  Func next("next");
  next(si) = numerator(si) / denominator(si);

  try{
    Target target = get_target_from_environment();
    target.set_feature(Target::Debug);
    next.compile_to_static_library("TestHalide.c", {}, "Test", target);
  } catch (Halide::Error &e){
    std::cerr << "Halide Error: " << e.what() << std::endl;
    __throw_exception_again;
  } 
}

throws an internal error:

Halide Error: Internal Error at /home/halidenightly/build_bot/worker/halide-nightly-release_17-x86-64-linux-cmake/halide-source/src/CodeGen_LLVM.cpp:1293 triggered by user code at : Symbol not found: denominator.s1.rv$x
The following names are in scope:
{
  ::Test
  denominator
  next
  next.buffer
  next.device_dirty
  next.dimensions
  next.extent.0
  next.min.0
  next.stride.0
  next.type
  numerator
  numerator.s1.rv$x
  numerator.si.extent_realized
  t12
}


whilst I believe this would be a valid program.

I've attached the output when running this with HL_DEBUG_CODEGEN=2

out.txt

I'm using the latest released version 17.0.1.

@sakehl sakehl changed the title Getting InternalError with a combination of compute_with and reductions Getting InternalError with a combination of compute_with, and reductions that use functions with update definitions Mar 11, 2024
@abadams abadams added the bug label Mar 11, 2024
@abadams abadams self-assigned this Mar 12, 2024
@abadams
Copy link
Member

abadams commented Mar 12, 2024

Thanks for the simple repro. I found that it's really not possible to simplify this one much more. It looks like the bug is specifically caused by a compute_with on an RVar where there's a producer stage whose bounds will depend on that RVar, because it's scheduled inside the loop over the RVar.

@sakehl
Copy link
Contributor Author

sakehl commented Mar 12, 2024

Yes I had the same observation. It came from a more complex program, where even without an intermediate function like denominator_inter this occurred. Let me know if you need more information.

abadams added a commit that referenced this issue Mar 13, 2024
This PR fixes a bug in compute_with, and another bug I found while
fixing it (we could really use a compute_with fuzzer).

The first bug is that you can get into situations where the bounds of a
producer func will refer directly to the loop variable of a consumer
func, where the consumer is in a compute_with fused group. In main, that
loop variable may not be defined because fused loop names have been
rewritten to include the token ".fused.". This PR adds let stmts to
define it just inside the fused loop body.

The second bug is that not all parent loops in compute_with fused groups
were having their bounds expanded to cover the region to be computed of
all children, because the logic for deciding which loops to expand only
considered the non-specialized pure definition. So e.g. compute_with
applied to an update stage would fail to compute values of the child
Func where they do not overlap with the parent Func. This PR visits all
definitions of the parent Func of the fused group, instead of just the
unspecialized pure definition of the parent Func.

Fixes #8149
abadams added a commit that referenced this issue Mar 15, 2024
* Fix two compute_with bugs.

This PR fixes a bug in compute_with, and another bug I found while
fixing it (we could really use a compute_with fuzzer).

The first bug is that you can get into situations where the bounds of a
producer func will refer directly to the loop variable of a consumer
func, where the consumer is in a compute_with fused group. In main, that
loop variable may not be defined because fused loop names have been
rewritten to include the token ".fused.". This PR adds let stmts to
define it just inside the fused loop body.

The second bug is that not all parent loops in compute_with fused groups
were having their bounds expanded to cover the region to be computed of
all children, because the logic for deciding which loops to expand only
considered the non-specialized pure definition. So e.g. compute_with
applied to an update stage would fail to compute values of the child
Func where they do not overlap with the parent Func. This PR visits all
definitions of the parent Func of the fused group, instead of just the
unspecialized pure definition of the parent Func.

Fixes #8149

* clang-tidy
sakehl added a commit to sakehl/Halide that referenced this issue May 1, 2024
* Fix two compute_with bugs.

This PR fixes a bug in compute_with, and another bug I found while
fixing it (we could really use a compute_with fuzzer).

The first bug is that you can get into situations where the bounds of a
producer func will refer directly to the loop variable of a consumer
func, where the consumer is in a compute_with fused group. In main, that
loop variable may not be defined because fused loop names have been
rewritten to include the token ".fused.". This PR adds let stmts to
define it just inside the fused loop body.

The second bug is that not all parent loops in compute_with fused groups
were having their bounds expanded to cover the region to be computed of
all children, because the logic for deciding which loops to expand only
considered the non-specialized pure definition. So e.g. compute_with
applied to an update stage would fail to compute values of the child
Func where they do not overlap with the parent Func. This PR visits all
definitions of the parent Func of the fused group, instead of just the
unspecialized pure definition of the parent Func.

Fixes halide#8149

* clang-tidy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants