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.

Author: abadams

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");
         }
 

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

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<Call::IntrinsicOp intrin>
 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<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) {
@@ -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) {

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

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