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()

[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_fuzz OBJECT)  # INTERFACE ~> OBJECT
add_library(Halide::fuzz ALIAS Halide_fuzz)

target_sources(Halide_fuzz PRIVATE ExprInterpreter.cpp) # NEW

if (NOT HAVE_LIBFUZZER_FLAGS)
    # ...
    target_sources(Halide_fuzz PRIVATE halide_fuzz_main.cpp halide_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?