[mlir][sparse] Renaming `CreationPolicy` to `Policy`

[wrengr]

Summary

This change is mostly for brevity's sake; but it also paves the way for the Policy enum to be reuseable for other situations that require the same three-way semantics.

[Peiming]

mlir/lib/Dialect/SparseTensor/IR/Detail/Var.h

393

Since we (at least at the current moment) requires forward declaration (when needed) for the dim/lvl variables. Do we still need the Policy?

Or do you have the plan to completely get rid of forward declaration in the future?

[wrengr]

mlir/lib/Dialect/SparseTensor/IR/Detail/Var.h

393

The long-term plan is to get rid of the forward-decls (of level-vars; the symbol-vars will still be there, of course). In D155533 (DimLvlMapParser.cpp:181–207) I add a longer explanation of why the forward-decls cause problems and how we can avoid the need for the forward-decls. The original design of VarEnv, Policy, VarInfo/VarInfo::ID vs Var/Var::Num, etc was set up precisely to solve this issue without the need for forward-decls.

mlir/lib/Dialect/SparseTensor/IR/Detail/DimLvlMapParser.h

public:
   explicit DimLvlMapParser(AsmParser &parser) : parser(parser) {}
 
   // Parses the input for a sparse tensor dimension-level map
   // and returns the map on success.
   FailureOr<DimLvlMap> parseDimLvlMap();
 
 private:
   OptionalParseResult parseVar(VarKind vk, bool isOptional,
-                               CreationPolicy creationPolicy, VarInfo::ID &id,
+                               Policy creationPolicy, VarInfo::ID &id,
                                bool &didCreate);
   FailureOr<VarInfo::ID> parseVarUsage(VarKind vk);
   FailureOr<std::pair<Var, bool>> parseVarBinding(VarKind vk, bool isOptional);
   FailureOr<Var> parseLvlVarBinding(bool directAffine);
 
   ParseResult parseOptionalIdList(VarKind vk, OpAsmParser::Delimiter delimiter);
   ParseResult parseDimSpec();
   ParseResult parseDimSpecList();

mlir/lib/Dialect/SparseTensor/IR/Detail/DimLvlMapParser.cpp

#define ERROR_IF(COND, MSG)                                                    \
   }
 
 //===----------------------------------------------------------------------===//
 // `DimLvlMapParser` implementation for variable parsing.
 //===----------------------------------------------------------------------===//
 
 // Our variation on `AffineParser::{parseBareIdExpr,parseIdentifierDefinition}`
 OptionalParseResult DimLvlMapParser::parseVar(VarKind vk, bool isOptional,
-                                              CreationPolicy creationPolicy,
+                                              Policy creationPolicy,
                                               VarInfo::ID &varID,
                                               bool &didCreate) {
   // Save the current location so that we can have error messages point to
   // the right place.  Note that `Parser::emitWrongTokenError` starts off
   // with the same location as `AsmParserImpl::getCurrentLocation` returns;
   // however, `Parser` will then do some various munging with the location
   // before actually using it, so `AsmParser::emitError` can't quite be used
   // as a drop-in replacement for `Parser::emitWrongTokenError`.
OptionalParseResult DimLvlMapParser::parseVar(VarKind vk, bool isOptional,
   if (const auto res = env.lookupOrCreate(creationPolicy, name, loc, vk)) {
     varID = res->first;
     didCreate = res->second;
     return success();
   }
   // TODO(wrengr): these error messages make sense for our intended usage,
   // but not in general; but it's unclear how best to factor that part out.
   switch (creationPolicy) {
-  case CreationPolicy::MustNot:
+  case Policy::MustNot:
     return parser.emitError(loc, "use of undeclared identifier '" + name + "'");
-  case CreationPolicy::May:
-    llvm_unreachable("got nullopt for CreationPolicy::May");
-  case CreationPolicy::Must:
+  case Policy::May:
+    llvm_unreachable("got nullopt for Policy::May");
+  case Policy::Must:
     return parser.emitError(loc, "redefinition of identifier '" + name + "'");
   }
-  llvm_unreachable("unknown CreationPolicy");
+  llvm_unreachable("unknown Policy");
 }
 
 FailureOr<VarInfo::ID> DimLvlMapParser::parseVarUsage(VarKind vk) {
   VarInfo::ID varID;
   bool didCreate;
-  const auto res = parseVar(vk, /*isOptional=*/false, CreationPolicy::MustNot,
-                            varID, didCreate);
+  const auto res =
+      parseVar(vk, /*isOptional=*/false, Policy::MustNot, varID, didCreate);
   if (!res.has_value() || failed(*res))
     return failure();
   return varID;
 }
 
 FailureOr<std::pair<Var, bool>>
 DimLvlMapParser::parseVarBinding(VarKind vk, bool isOptional) {
   VarInfo::ID id;
   bool didCreate;
-  const auto res =
-      parseVar(vk, isOptional, CreationPolicy::Must, id, didCreate);
+  const auto res = parseVar(vk, isOptional, Policy::Must, id, didCreate);
   if (res.has_value()) {
     FAILURE_IF_FAILED(*res)
     return std::make_pair(env.bindVar(id), true);
   }
   return std::make_pair(env.bindUnusedVar(vk), false);
 }
 
 FailureOr<Var> DimLvlMapParser::parseLvlVarBinding(bool directAffine) {

mlir/lib/Dialect/SparseTensor/IR/Detail/Var.h

public:
   }
 };
 // We don't actually require this, since `VarInfo` is a proper struct
 // rather than a newtype.  But it passes, so for now we'll keep it around.
 // TODO: Uncomment the static assert, it fails the build with gcc7 right now.
 // static_assert(IsZeroCostAbstraction<VarInfo>);
 
 //===----------------------------------------------------------------------===//
-enum class CreationPolicy { MustNot, May, Must };
+enum class Policy { MustNot, May, Must };
 
+//===----------------------------------------------------------------------===//
 class VarEnv final {
   /// Map from `VarKind` to the next free `Var::Num`; used by `bindVar`.
   VarKindArray<Var::Num> nextNum;
   /// Map from `VarInfo::ID` to shared storage for the actual `VarInfo` objects.
   SmallVector<VarInfo> vars;
   /// Map from variable names to their `VarInfo::ID`.
   llvm::StringMap<VarInfo::ID> ids;
 
public:
   /// when `verifyUsage` is false, this is a noop.  Returns the identifier
   /// for the variable with the given name (i.e., either the newly created
   /// variable, or the pre-existing variable), and a bool indicating whether
   /// a new variable was created.
   std::pair<VarInfo::ID, bool> create(StringRef name, llvm::SMLoc loc,
                                       VarKind vk, bool verifyUsage = false);
 
   /// Attempts to lookup or create a variable according to the given
-  /// `CreationPolicy`.  Returns nullopt in one of two circumstances:
+  /// `Policy`.  Returns nullopt in one of two circumstances:
   /// (1) the policy says we `Must` create, yet the variable already exists;
   /// (2) the policy says we `MustNot` create, yet no such variable exists.
   /// Otherwise, if the variable already exists then it is validated against
   /// the given kind and location to ensure consistency.
   //
   // TODO(wrengr): Define an enum of error codes, to avoid `nullopt`-blindness
   // TODO(wrengr): Prolly want to rename this to `create` and move the
   // current method of that name to being a private `createImpl`.
   std::optional<std::pair<VarInfo::ID, bool>>
-  lookupOrCreate(CreationPolicy policy, StringRef name, llvm::SMLoc loc,
+  lookupOrCreate(Policy creationPolicy, StringRef name, llvm::SMLoc loc,
                  VarKind vk);
 
   /// Binds the given variable to the next free `Var::Num` for its `VarKind`.
   Var bindVar(VarInfo::ID id);
 
   /// Creates a new variable of the given kind and immediately binds it.
   /// This should only be used whenever the variable is known to be unused
   /// and therefore does not have a name.

mlir/lib/Dialect/SparseTensor/IR/Detail/Var.cpp

#ifndef NDEBUG
     if (verifyUsage)
       assertUsageConsistency(*this, id, loc, vk);
 #endif // NDEBUG
   }
   return std::make_pair(id, didInsert);
 }
 
 std::optional<std::pair<VarInfo::ID, bool>>
-VarEnv::lookupOrCreate(CreationPolicy policy, StringRef name, llvm::SMLoc loc,
+VarEnv::lookupOrCreate(Policy creationPolicy, StringRef name, llvm::SMLoc loc,
                        VarKind vk) {
-  switch (policy) {
-  case CreationPolicy::MustNot: {
+  switch (creationPolicy) {
+  case Policy::MustNot: {
     const auto oid = lookup(name);
     if (!oid)
       return std::nullopt; // Doesn't exist, but must not create.
 #ifndef NDEBUG
     assertUsageConsistency(*this, *oid, loc, vk);
 #endif // NDEBUG
     return std::make_pair(*oid, false);
   }
-  case CreationPolicy::May:
+  case Policy::May:
     return create(name, loc, vk, /*verifyUsage=*/true);
-  case CreationPolicy::Must: {
+  case Policy::Must: {
     const auto res = create(name, loc, vk, /*verifyUsage=*/false);
     // const auto id = res.first;
     const auto didCreate = res.second;
     if (!didCreate)
       return std::nullopt; // Already exists, but must create.
     return res;
   }
   }
-  llvm_unreachable("unknown CreationPolicy");
+  llvm_unreachable("unknown Policy");
 }
 
 Var VarEnv::bindUnusedVar(VarKind vk) { return Var(vk, nextNum[vk]++); }
 Var VarEnv::bindVar(VarInfo::ID id) {
   auto &info = access(id);
   const auto var = bindUnusedVar(info.getKind());
   // NOTE: `setNum` already checks wellformedness of the `Var::Num`.
   info.setNum(var.getNum());