ExprInterpreter.

[mcourteaux]

Second step in addressing #9044.

[abadams]

This should just be:

if (op->is_strict_intrinsic()) {
Expr lowered = unstrictify_float(op);
lowered.accept(this);
}

Also instead of interpreting the fixed-point instrinsics (e.g. halving add), in the else case at the bottom you can call lower_intrinsic(op) and try again.

[mcourteaux]

I'll leave strict_fma and redirect it to std::fma().

[mcourteaux]

There is no helper function to tell if an intrinsic is a fixed-point intrinsic.

[mcourteaux]

Added a helper: is_integer_intrinsic()

[mcourteaux]

Renamed the helper to is_arithmetic_intrinsic().

[abadams]

I think the interpreter should be in test/fuzz, until such time as we need it more generally. is_integer_intrinsic isn't quite right as a name because you can indeed do widening_mul on floats. lower_intrinsic (the existing name) is also not great because it's too broad. Maybe we should call these "arithmetic intrinsics" and have is_arithmetic_intrinsic and lower_arithmetic_intrinsic? Open to ideas for a better name.

[abadams]

I don't think this implements Halide math correctly. E.g. there's no overflow handling on integer math, and you don't get the same rounding behavior if you emulate a narrow float with a double. We may want to reuse existing compiler machinery (e.g. the constant folding in IRMatch.h), but that means we're not testing that logic. Let's discuss the best way to do this at the dev meeting.

[mcourteaux]

I think the interpreter should be in test/fuzz, until such time as we need it more generally.

The tests don't support linking multiple .cpp files together. We could #include "ExprInterpreter.cpp" which is totally valid and the easiest, but perhaps not the nicest.

Maybe we should call these "arithmetic intrinsics" and have is_arithmetic_intrinsic and lower_arithmetic_intrinsic? Open to ideas for a better name.

That sounds like it would include all strict_float intrinsics too.

you can indeed do widening_mul on floats.

It's possible because widen() is generic enough. But it seems like it's intended for int-types only.

I don't think this implements Halide math correctly. E.g. there's no overflow handling on integer math, and you don't get the same rounding behavior if you emulate a narrow float with a double.

I wonder if that's strictly required for our fuzzing purpose. We want to test the simplifier, CodeGen backends, and LLVM. We can at least test it for all the normal types (f32, f64, i/u 1,8,16,32,64) if we don't trigger overflows with our test numbers. Having this rudimentary ExprInterpreter sit in the test/fuzz/ folder seems like it's clear enough that it's for testing purposes.

there's no overflow handling on integer math
Where are the rules in Halide for this anyway? I currently don't know about any overflow handling in Halide.

[alexreinking]

The tests don't support linking multiple .cpp files together. We could #include "ExprInterpreter.cpp" which is totally valid and the easiest, but perhaps not the nicest.

It's not that bad... add it to the Halide_fuzz target in CMake. Something like:

# Existing code:add_library(Halide_fuzzOBJECT) # INTERFACE ~> OBJECTadd_library(Halide::fuzzALIASHalide_fuzz)
target_sources(Halide_fuzzPRIVATEExprInterpreter.cpp) # NEWif (NOT HAVE_LIBFUZZER_FLAGS)
# ...target_sources(Halide_fuzzPRIVATEhalide_fuzz_main.cpphalide_fuzz_main.h) # INTERFACE ~> PRIVATE# ...

Adjusts two lines, adds a third.

I wonder if that's strictly required for our fuzzing purpose. We want to test the simplifier, CodeGen backends, and LLVM. We can at least test it for all the normal types (f32, f64, i/u 1,8,16,32,64) if we don't trigger overflows with our test numbers. Having this rudimentary ExprInterpreter sit in the test/fuzz/ folder seems like it's clear enough that it's for testing purposes.

We do promise defined overflow on signed ints narrower than 32.

[abadams]

Also uints have the normal wrapping behavior. As soon as you subtract two numbers, you're going to get overflow for the uint types, so it does need to be handled.

The strict_float intrinsics are different, because "lowering" them does sort of change the meaning of the Expr, in that it relaxes the guarantees. For the others, it should be exactly identical. widening_mul(f16, f16) -> f32 is very much intended. I can't think of a good alternative to arithmetic right now but I'll keep thinking.

[mcourteaux]

I can't think of a good alternative to arithmetic right now but I'll keep thinking.

I did like the name. I was just thinking that strict_add and strict_mul etc are also arithmetic operations. Was wondering if they should be included in the list for is_arithmetic_intrinsic() or we should go for is_non_strict_arithmetic_intrinsic(), or just a documentation note saying they are excluded.

We do promise defined overflow on signed ints narrower than 32.

So the concern here is that C++ doesn't consider this as defined behavior, and we do. So, despite the fact that all of the code will most likely just work, it's technically still UB.

Also uints have the normal wrapping behavior. As soon as you subtract two numbers, you're going to get overflow for the uint types, so it does need to be handled.

C++ already defines wraparound with module 2^N for unsigned integer types. So I think the expression interpreter should already do this automatically. The problem would only exist for signed integers (32 bits or narrower), because it's undefined in C++.
Thinking more about this, I think it might already be handled: the std::variant uses int64_t to handle all signed integers types, followed by a truncation to the desired bit-width. No arithmetic op will ever overflow the int64_t for int32_t operands.

add it to the Halide_fuzz target in CMake

Great, I'll do that! One unfortunate side effect of this is that the call to ExprInterpreter::test() is no longer valid from within test/_internal.cpp. How would we now call the this internal test?

[alexreinking]

One unfortunate side effect of this is that the call to ExprInterpreter::test() is no longer valid from within test/_internal.cpp. How would we now call the this internal test?

It will be tested plenty, indirectly by the fuzz tests.

[mcourteaux]

@abadams Can you do the fix-up for the Makefile? ExprInterpreter.cpp should be part of the fuzzing targets.

[mcourteaux]

I don't think this implements Halide math correctly. E.g. there's no overflow handling on integer math, and you don't get the same rounding behavior if you emulate a narrow float with a double. We may want to reuse existing compiler machinery (e.g. the constant folding in IRMatch.h), but that means we're not testing that logic. Let's discuss the best way to do this at the dev meeting.

I think I fully implemented overflow handling as defined by Halide rules.

[abadams]

Failures in makefile are now warnings in ExprInterpreter.cpp that should probably be fixed

[abadams]

I think this should mostly replace the samples loop above, instead of happening as well. The loop above checks other things too though (idempotence), so a little refactoring is needed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.05%. Comparing base (03a18bb) to head (85c6e87).
⚠️ Report is 65 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #9047 +/- ##
==========================================
+ Coverage 69.83% 70.05% +0.22% 
==========================================
Files 258 261 +3 Lines 78210 79426 +1216 Branches 19037 19362 +325 ==========================================
+ Hits 54617 55644 +1027 - Misses 17832 17905 +73 - Partials 5761 5877 +116 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.