Add filtering capabilities to HL_DEBUG_CODEGEN

[alexreinking]

This PR extends the HL_DEBUG_CODEGEN semantics with features for filtering messages only from certain files, functions, and lines. I'm opening this as a draft to invite discussion.

The current syntax for HL_DEBUG_CODEGEN is a single number setting the verbosity level globally for all debug(N) streams.

The proposed syntax for HL_DEBUG_CODEGEN is a ;-delimited list of rules of the form:

verbosity[,file_suffix[:line_low[-line_high]]][@function_suffix]

• verbosity is a positive integer indicating the maximum (inclusive) verbosity level to emit. This is equivalent to the current semantics
• file_suffix is a maximal string of non-:@ characters. If it is specified, the rule applies only to files ending in this string. Typically, this will be a path fragment like src/Func.cpp.
• The values line_low and line_high specify a range of line numbers, inclusive on both sides. If neither is specified, they are 0 and INT_MAX. If only line_low is specified, line_high is set equal to line_low.
• function_suffix is a string. If it is specified, the rule applies only to functions (as determined by __FUNCTION__) whose names end in this string. Typically, this will be the whole name of a function (e.g. rfactor)

The motivation for this is to enable higher-signal print debugging. For example, a useful setting for me while debugging rfactor might be: 1;5,Func.cpp@rfactor;5,Associativity.cpp

[mcourteaux]

I like the approach! I have two questions:

Why you chose for suffixes on filename and function name, instead of prefix, or just "contains" the test string.

For file paths, I'm guessing prefix is impractical because you'd have the full path of the file (as opposed to the name). Same goes for "contains", as you might accidentally match the whole source tree if you have a parent path somewhere that contains the target string.

But, for function names, I don't see a good reason why it should be a suffix match.

Another question I have is, can you specify a function name without specifying a file name?
Something like this might work?

verbosity[,filename[:line_low[-line_high]]][@func]

AFAIK paths can't contain commas nor colons. First @ signals begin of function part of the rule. Like this, line numbers are close to the filename, as is more common. I think it supports the same functionality, but now we can be lazy, and immediately go to the function part of it:

1;5@rfactor

for maximal laziness (and potentially an even wider match across the project).

[alexreinking]

Why you chose for suffixes on filename and function name, instead of prefix, or just "contains" the test string.

Contains might be better, but suffix was quick to implement because of ends_with in Util.hpp. It's also more useful than a prefix since we're matching against the full path (e.g./Users/areinking/dev/Halide/src/Func.cpp)

verbosity[,filename[:line_low[-line_high]]][@func]

That's a nicer syntax, I agree. I'll update the PR. 🙂

[mcourteaux]

Seems that c805e54 is biting you on MSVC.

[alexreinking]

Seems that c805e54 is biting you on MSVC.

That build failure is thanks to a missing <algorithm> include. But we should disable that warning for ErrorReport with a pragma.

[alexreinking]

I'm realizing that debug(1) << foo;, unlike internal_assert(x) << foo, evaluates foo no matter what. In contrast, internal_assert only evaluates foo if x is false. Should we introduce this behavior to debug? It would let me get rid of the if constexpr hack and use IIFE as I originally intended.

[mcourteaux]

I'm realizing that debug(1) << foo;, unlike internal_assert(x) << foo, evaluates foo no matter what. In contrast, internal_assert only evaluates foo if x is false. Should we introduce this behavior to debug? It would let me get rid of the if constexpr hack and use IIFE as I originally intended.

I am in favor of not having foo be evaluated. Correctness should not be dependent on a debug print. 😝 It would probably also save a bit of performance. There is quite some debug() << throughout the codebase.

[alexreinking]

With the voidifier trick, I managed to remove the debug class entirely. Now it's just a free function and a macro that conditionally evaluates to std::cerr.

[alexreinking]

There are still two "client" uses of debug_is_active. One is in Simplify.cpp and the other is in SimplifyCorrelatedDifferences.cpp. In both cases, they are guarding (potentially expensive?) checks, for failed proofs and non-monotonicity, respectively. I wonder if there's a better thing to do here.

The only other use is in the ReportBase for error handling, where source position information is included only when the debug level is at least 1.

[steven-johnson]

Generally this looks awesome, but see my comment about debug leaking into non-Halide code and issues thereof

[alexreinking]

@abadams -- how should we pipe -DHALIDE_KEEP_MACROS to the tests in the Makefile?

[alexreinking]

This is ready for review.

[mcourteaux]

Super useful! Thanks for this. I'm using it two days straight now, and it's very very handy! 😄