Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle some misc TODOs #8528

Merged
merged 1 commit into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/CodeGen_LLVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

Expand Down
11 changes: 9 additions & 2 deletions src/Deinterleave.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

Expand All @@ -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();
}
Expand Down
9 changes: 2 additions & 7 deletions src/IRMatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<Call::IntrinsicOp intrin>
struct OptionalIntrinType {
bool check(const Type &) const {
Expand Down Expand Up @@ -1413,7 +1408,7 @@ struct Intrin {
return saturating_cast(optional_type_hint.type, std::move(arg0));
}

Expr arg1 = std::get<const_min(1, sizeof...(Args) - 1)>(args).make(state, type_hint);
Expr arg1 = std::get<std::min<size_t>(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) {
Expand Down Expand Up @@ -1448,7 +1443,7 @@ struct Intrin {
return rounding_shift_right(std::move(arg0), std::move(arg1));
}

Expr arg2 = std::get<const_min(2, sizeof...(Args) - 1)>(args).make(state, type_hint);
Expr arg2 = std::get<std::min<size_t>(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) {
Expand Down
32 changes: 12 additions & 20 deletions src/IROperator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<Call::IntrinsicOp> &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<Call::IntrinsicOp> &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) {
Expand Down
7 changes: 3 additions & 4 deletions src/Var.h
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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);
Expand Down