From e34b279be30764f1bb25993ee3325be157931fe0 Mon Sep 17 00:00:00 2001 From: Z Stern Date: Mon, 15 Jul 2019 14:06:08 -0700 Subject: [PATCH 1/2] Attempt to fix issue #3813. --- src/CSE.cpp | 4 +++ src/CSE.h | 5 ++++ src/Function.cpp | 6 ++-- src/StrictifyFloat.cpp | 17 +++++++---- src/StrictifyFloat.h | 8 ++++- test/correctness/strict_float.cpp | 50 +++++++++++++++++++++++++++++++ 6 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/CSE.cpp b/src/CSE.cpp index 5539df52ab21..d87b182eac46 100644 --- a/src/CSE.cpp +++ b/src/CSE.cpp @@ -6,6 +6,7 @@ #include "IROperator.h" #include "Scope.h" #include "Simplify.h" +#include "StrictifyFloat.h" namespace Halide { namespace Internal { @@ -294,6 +295,9 @@ Stmt common_subexpression_elimination(const Stmt &s, bool lift_all) { return CSEEveryExprInStmt(lift_all).mutate(s); } +Expr common_subexpression_elimination_front_end(const Expr &e_in, bool lift_all) { + return common_subexpression_elimination(strictify_float(e_in), lift_all); +} // Testing code. diff --git a/src/CSE.h b/src/CSE.h index 23e1067b588e..2290a7bc6d88 100644 --- a/src/CSE.h +++ b/src/CSE.h @@ -29,6 +29,11 @@ Expr common_subexpression_elimination(const Expr &, bool lift_all = false); * statement. Does not introduce let statements. */ Stmt common_subexpression_elimination(const Stmt &, bool lift_all = false); +/** A version of common_subexpression_elimination that can be called before + * certain steps in lowering. Handles strict_float and certain issues of variable + * naming. */ +Expr common_subexpression_elimination_front_end(const Expr &, bool lift_all = false); + void cse_test(); } // namespace Internal diff --git a/src/Function.cpp b/src/Function.cpp index 02e5e2d1b77e..2db15e7ffe2d 100644 --- a/src/Function.cpp +++ b/src/Function.cpp @@ -414,7 +414,7 @@ void Function::define(const vector &args, vector values) { } for (size_t i = 0; i < values.size(); i++) { - values[i] = common_subexpression_elimination(values[i]); + values[i] = common_subexpression_elimination_front_end(values[i]); } // Tag calls to random() with the free vars @@ -523,12 +523,12 @@ void Function::define_update(const vector &_args, vector values) { err << values[i].type() << ", but pure definition has type " << pure_type; user_error << err.str() << "\n"; } - values[i] = common_subexpression_elimination(values[i]); + values[i] = common_subexpression_elimination_front_end(values[i]); } vector args(_args.size()); for (size_t i = 0; i < args.size(); i++) { - args[i] = common_subexpression_elimination(_args[i]); + args[i] = common_subexpression_elimination_front_end(_args[i]); } // The pure args are those naked vars in the args that are not in diff --git a/src/StrictifyFloat.cpp b/src/StrictifyFloat.cpp index 96db4b2135ce..9fe928907ed7 100644 --- a/src/StrictifyFloat.cpp +++ b/src/StrictifyFloat.cpp @@ -6,14 +6,14 @@ namespace Halide { namespace Internal { -class StrictifyFloat : public IRMutator { +class StrictifyFloat : public IRGraphMutator2 { bool strict_float_allowed; enum Strictness { FastMath, StrictFloat, } strictness; - using IRMutator::visit; + using IRGraphMutator2::visit; Expr visit(const Call *call) override { Strictness new_strictness = strictness; @@ -26,16 +26,17 @@ class StrictifyFloat : public IRMutator { ScopedValue save_strictness(strictness, new_strictness); - return IRMutator::visit(call); + return IRGraphMutator2::visit(call); } - using IRMutator::mutate; + using IRGraphMutator2::mutate; +public: Expr mutate(const Expr &expr) override { if (!expr.defined()) { return expr; } - Expr e = IRMutator::mutate(expr); + Expr e = IRGraphMutator2::mutate(expr); if (e.type().is_float()) { switch (strictness) { case FastMath: @@ -47,7 +48,6 @@ class StrictifyFloat : public IRMutator { return e; } -public: enum StrictnessMode { NotAllowed, Allowed, @@ -80,5 +80,10 @@ bool strictify_float(std::map &env, const Target &t) { return any_strict_float; } +Expr strictify_float(Expr e) { + StrictifyFloat strictify(StrictifyFloat::Allowed); + return strictify.mutate(e); +} + } // namespace Internal } // namespace Halide diff --git a/src/StrictifyFloat.h b/src/StrictifyFloat.h index 8ca305dd8251..b658dc891f52 100644 --- a/src/StrictifyFloat.h +++ b/src/StrictifyFloat.h @@ -15,13 +15,19 @@ namespace Internal { /** Propagate strict_float intrinisics such that they immediately wrap * all floating-point expressions. This makes the IR nodes context - * independent. If the Target::StrictFloat flag is specified in + * independent. If the Target::StrictFloat flag is specified in * target, starts in strict_float mode so all floating-point type * Exprs in the compilation will be marked with strict_float. Returns * whether any strict floating-point is used in any function in the * passed in env. + * + * Expr version applies to a single Expr and is required for use in + * frontend IR construction occassionally. */ +// @{ bool strictify_float(std::map &env, const Target &t); +Expr strictify_float(Expr e); +// @} } // namespace Internal } // namespace Halide diff --git a/test/correctness/strict_float.cpp b/test/correctness/strict_float.cpp index e08d22cc0931..a31f3855a833 100644 --- a/test/correctness/strict_float.cpp +++ b/test/correctness/strict_float.cpp @@ -4,6 +4,7 @@ #include #include #include +#include "HalideBuffer.h" using namespace Halide; @@ -286,6 +287,55 @@ int main(int argc, char **argv) { in.set(transposed); run_all_conditions("sorted descending transposed", transposed); + // TODO: This needs to be made in to a test. Currently it is not + // reproducing the reported failure however because both max_diff_offset and + // max_diff_strict_offset are zero. + // + // Check case from reported bug where CSE in frontend was breaking strict_float. + // (See: https://github.com/halide/Halide/issues/3813) + Var x, y; + Func f, f_offset, f_strict, f_strict_offset; + Expr sval_mul = 43758.5453123f * sin(y * 78.233f + x * 12.9898f); + Expr sval_mul_offset = 43758.5453123f * sin(y * 78.233f + (x + 1) * 12.9898f); + Expr rand_val = sval_mul - floor(sval_mul); + Expr rand_val_offset = sval_mul_offset - floor(sval_mul_offset); + f(x, y) = rand_val; + f_offset(x, y) = rand_val_offset; + f_strict(x, y) = strict_float(rand_val); + f_strict_offset(x, y) = strict_float(rand_val_offset); + + Buffer result = f.realize(513, 512); + Buffer result_offset = f_offset.realize(512, 512); + Buffer result_strict = f_strict.realize(513, 512); + Buffer result_strict_offset = f_strict_offset.realize(512, 512); + + float max_diff = 0.0f; + float max_diff_offset = 0.0f; + float max_diff_strict_offset = 0.0f; + for (int32_t y = 0; y < 512; y++) { + for (int32_t x = 0; x < 512; x++) { + float diff = fabs(result(x, y) - result_strict(x, y)); + float diff_offset = fabs(result(x + 1, y) - result_offset(x, y)); + float diff_strict_offset = fabs(result_strict(x + 1, y) - result_strict_offset(x, y)); + + if (diff > max_diff) { + max_diff = diff; + } + if (diff_offset > max_diff_offset) { + max_diff_offset = diff_offset; + } + if (diff_strict_offset > max_diff_strict_offset) { + max_diff_strict_offset = diff_strict_offset; + } + } + } + printf("Max diff %f max diff offset %f max diff strict offset %f.\n", max_diff, max_diff_offset, max_diff_strict_offset); + + // TODO: Get first one to fail to demonstrate bug. Only second one + // should persist and it should get a tolerance. + assert(max_diff_offset == 0.0f); + assert(max_diff_strict_offset == 0.0f); + printf("Success!\n"); return 0; From c9c211b1fcdc41a0ff764c9c8c07e48203266e9b Mon Sep 17 00:00:00 2001 From: Z Stern Date: Wed, 17 Jul 2019 13:42:26 -0700 Subject: [PATCH 2/2] Fix strict float to not duplicate strict_float wrappings. --- src/StrictifyFloat.cpp | 77 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 14 deletions(-) diff --git a/src/StrictifyFloat.cpp b/src/StrictifyFloat.cpp index 9fe928907ed7..cdd18f18cd4e 100644 --- a/src/StrictifyFloat.cpp +++ b/src/StrictifyFloat.cpp @@ -6,46 +6,95 @@ namespace Halide { namespace Internal { -class StrictifyFloat : public IRGraphMutator2 { +class StrictifyFloat : public IRMutator { bool strict_float_allowed; enum Strictness { FastMath, + StrictFloatFirst, StrictFloat, } strictness; - using IRGraphMutator2::visit; + struct StrictnessExprCompare { + bool operator()(const std::pair &a, + const std::pair &b) const { + return ((int)a.first < (int)b.first) || + (a.first == b.first && (a.second.get() < b.second.get())); + } + }; + + struct StrictnessStmtCompare { + bool operator()(const std::pair &a, + const std::pair &b) const { + return ((int)a.first < (int)b.first) || + (a.first == b.first && Stmt::Compare()(a.second, b.second)); + } + }; + + std::map, Expr, StrictnessExprCompare> expr_replacements; + std::map, Stmt, StrictnessStmtCompare> stmt_replacements; + + using IRMutator::visit; Expr visit(const Call *call) override { Strictness new_strictness = strictness; if (call->is_intrinsic(Call::strict_float)) { user_assert(strict_float_allowed) << "strict_float intrinsic is not allowed unless target has feature 'allow_strict_float' or 'force_strict_float'\n"; - new_strictness = StrictFloat; + new_strictness = StrictFloatFirst; any_strict_float |= true; } ScopedValue save_strictness(strictness, new_strictness); - return IRGraphMutator2::visit(call); + return IRMutator::visit(call); } - using IRGraphMutator2::mutate; - public: Expr mutate(const Expr &expr) override { if (!expr.defined()) { return expr; } - Expr e = IRGraphMutator2::mutate(expr); - if (e.type().is_float()) { - switch (strictness) { - case FastMath: - return e; - case StrictFloat: - return strict_float(e); + + auto p = expr_replacements.emplace(std::make_pair(strictness, expr), Expr()); + if (p.second) { + Expr e; + { + Strictness new_strictness = (strictness == StrictFloatFirst) ? StrictFloat: strictness; + ScopedValue save_strictness(strictness, new_strictness); + + e = IRMutator::mutate(expr); + } + + if (e.type().is_float()) { + switch (strictness) { + case FastMath: + case StrictFloatFirst: + break; + case StrictFloat: + const Call *call = e.as(); + if (call == nullptr || !call->is_intrinsic(Call::strict_float)) { + e = strict_float(e); + } + break; + } } + p.first->second = std::move(e); + } + + return p.first->second; + } + + Stmt mutate(const Stmt &s) override { + if (!s.defined()) { + return s; + } + auto p = stmt_replacements.emplace(std::make_pair(strictness, s), Stmt()); + if (p.second) { + // N.B: Inserting into a map (as the recursive mutate call + // does), does not invalidate existing iterators. + p.first->second = IRMutator::mutate(s); } - return e; + return p.first->second; } enum StrictnessMode {