From bc9dfbf99de9d52578d28967a31aad71104e068a Mon Sep 17 00:00:00 2001 From: Andrew Adams Date: Fri, 20 Dec 2024 09:42:51 -0800 Subject: [PATCH] Handle some misc TODOs (#8528) CodeGen_LLVM.cpp: opaque pointers are now standard, and that flag no longer works. Var.h: We convert strings to Vars in many places internally, and some of those Vars originated from implicit Vars, so it's not feasible to require than the version that takes an explicit string isn't allowed to be passed things of the form "_[0-9]*". You can use the explicit constructor to make collisions with other Vars, and yes this includes the implicit vars. --- src/CodeGen_LLVM.cpp | 3 --- src/Deinterleave.cpp | 11 +++++++++-- src/IRMatch.h | 9 ++------- src/IROperator.cpp | 32 ++++++++++++-------------------- src/Var.h | 7 +++---- 5 files changed, 26 insertions(+), 36 deletions(-) diff --git a/src/CodeGen_LLVM.cpp b/src/CodeGen_LLVM.cpp index 31dddc3551af..a30b30ab3276 100644 --- a/src/CodeGen_LLVM.cpp +++ b/src/CodeGen_LLVM.cpp @@ -234,9 +234,6 @@ void CodeGen_LLVM::initialize_llvm() { for (const std::string &s : arg_vec) { c_arg_vec.push_back(s.c_str()); } - // TODO: Remove after opaque pointers become the default in LLVM. - // This is here to document how to turn on opaque pointers, for testing, in LLVM 15 - // c_arg_vec.push_back("-opaque-pointers"); cl::ParseCommandLineOptions((int)(c_arg_vec.size()), &c_arg_vec[0], "Halide compiler\n"); } diff --git a/src/Deinterleave.cpp b/src/Deinterleave.cpp index 255b2a67a339..54199485fed5 100644 --- a/src/Deinterleave.cpp +++ b/src/Deinterleave.cpp @@ -223,7 +223,7 @@ class Deinterleaver : public IRGraphMutator { } } if (op->value.type().lanes() > 1) { - // There is probably a more efficient way to this. + // There is probably a more efficient way to do this. return mutate(flatten_nested_ramps(op)); } @@ -236,7 +236,14 @@ class Deinterleaver : public IRGraphMutator { } else { Type t = op->type.with_lanes(new_lanes); ModulusRemainder align = op->alignment; - // TODO: Figure out the alignment of every nth lane + // The alignment of a Load refers to the alignment of the first + // lane, so we can preserve the existing alignment metadata if the + // deinterleave is asking for any subset of lanes that includes the + // first. Otherwise we just drop it. We could check if the index is + // a ramp with constant stride or some other special case, but if + // that's the case, the simplifier is very good at figuring out the + // alignment, and it has access to context (e.g. the alignment of + // enclosing lets) that we do not have here. if (starting_lane != 0) { align = ModulusRemainder(); } diff --git a/src/IRMatch.h b/src/IRMatch.h index afc264582651..676bd037bd27 100644 --- a/src/IRMatch.h +++ b/src/IRMatch.h @@ -1320,11 +1320,6 @@ constexpr bool and_reduce(bool first, Args... rest) { return first && and_reduce(rest...); } -// TODO: this can be replaced with std::min() once we require C++14 or later -constexpr int const_min(int a, int b) { - return a < b ? a : b; -} - template struct OptionalIntrinType { bool check(const Type &) const { @@ -1413,7 +1408,7 @@ struct Intrin { return saturating_cast(optional_type_hint.type, std::move(arg0)); } - Expr arg1 = std::get(args).make(state, type_hint); + Expr arg1 = std::get(1, sizeof...(Args) - 1)>(args).make(state, type_hint); if (intrin == Call::absd) { return absd(std::move(arg0), std::move(arg1)); } else if (intrin == Call::widen_right_add) { @@ -1448,7 +1443,7 @@ struct Intrin { return rounding_shift_right(std::move(arg0), std::move(arg1)); } - Expr arg2 = std::get(args).make(state, type_hint); + Expr arg2 = std::get(2, sizeof...(Args) - 1)>(args).make(state, type_hint); if (intrin == Call::mul_shift_right) { return mul_shift_right(std::move(arg0), std::move(arg1), std::move(arg2)); } else if (intrin == Call::rounding_mul_shift_right) { diff --git a/src/IROperator.cpp b/src/IROperator.cpp index 41ea946e10f4..26869c4d8b15 100644 --- a/src/IROperator.cpp +++ b/src/IROperator.cpp @@ -1048,49 +1048,41 @@ Expr strided_ramp_base(const Expr &e, int stride) { namespace { -struct RemoveLikelies : public IRMutator { +// Replace a specified list of intrinsics with their first arg. +class RemoveIntrinsics : public IRMutator { using IRMutator::visit; + const std::initializer_list &ops; + Expr visit(const Call *op) override { - if (op->is_intrinsic(Call::likely) || - op->is_intrinsic(Call::likely_if_innermost)) { + if (op->is_intrinsic(ops)) { return mutate(op->args[0]); } else { return IRMutator::visit(op); } } -}; -// TODO: There could just be one IRMutator that can remove -// calls from a list. If we need more of these, it might be worth -// doing that refactor. -struct RemovePromises : public IRMutator { - using IRMutator::visit; - Expr visit(const Call *op) override { - if (op->is_intrinsic(Call::promise_clamped) || - op->is_intrinsic(Call::unsafe_promise_clamped)) { - return mutate(op->args[0]); - } else { - return IRMutator::visit(op); - } +public: + RemoveIntrinsics(const std::initializer_list &ops) + : ops(ops) { } }; } // namespace Expr remove_likelies(const Expr &e) { - return RemoveLikelies().mutate(e); + return RemoveIntrinsics({Call::likely, Call::likely_if_innermost}).mutate(e); } Stmt remove_likelies(const Stmt &s) { - return RemoveLikelies().mutate(s); + return RemoveIntrinsics({Call::likely, Call::likely_if_innermost}).mutate(s); } Expr remove_promises(const Expr &e) { - return RemovePromises().mutate(e); + return RemoveIntrinsics({Call::promise_clamped, Call::unsafe_promise_clamped}).mutate(e); } Stmt remove_promises(const Stmt &s) { - return RemovePromises().mutate(s); + return RemoveIntrinsics({Call::promise_clamped, Call::unsafe_promise_clamped}).mutate(s); } Expr unwrap_tags(const Expr &e) { diff --git a/src/Var.h b/src/Var.h index 33207d3cea0d..fbb976b476d9 100644 --- a/src/Var.h +++ b/src/Var.h @@ -25,7 +25,9 @@ class Var { Expr e; public: - /** Construct a Var with the given name */ + /** Construct a Var with the given name. Unlike Funcs, this will be treated + * as the same Var as another other Var with the same name, including + * implicit Vars. */ Var(const std::string &n); /** Construct a Var with an automatically-generated unique name. */ @@ -120,9 +122,6 @@ class Var { static Var implicit(int n); /** Return whether a variable name is of the form for an implicit argument. - * TODO: This is almost guaranteed to incorrectly fire on user - * declared variables at some point. We should likely prevent - * user Var declarations from making names of this form. */ //{ static bool is_implicit(const std::string &name);