Teach the MLIR AsmPrinter to correctly escape asm names that use invalid characters.

[rriddle]

What is the benefit of doing this vs. just rejecting/ignoring the name? Do we need to support the weird cases?

[rriddle]

In D75919#1915161, @rriddle wrote:

What is the benefit of doing this vs. just rejecting/ignoring the name? Do we need to support the weird cases?

Also, what about using a string as the escape mechanism? We already do this for other things.

%"a .^x" : i32

[lattner]

Who should own the complexity (and the definition of what a valid asm name is): the core, or all clients?

[lattner]

Also, what about using a string as the escape mechanism?

I'm open to this. This this what Swift does, where you can have an identifier x, or you can have an identifier x that allows a very generous grammar within it.

In this case, I don't think we want to promise to dialects that the textual IR will persist an arbitrary grammar. I think it is better to keep the asm format simple, and force dialects who care about this stuff (none of them today, and almost none of them in the future) to own the autorenaming complexity.

[lattner]

River, does this patch LGTY? It is pretty straight-forward.

[rriddle]

Sorry, I forgot to hit submit. Avoiding the use of string is fine for now, as it keeps this specific to this one part of the printer.

mlir/lib/IR/AsmPrinter.cpp

763

Please use /// for top-level comments.

788

nit: auto -> char?

[lattner]

Thanks River!

mlir/lib/IR/AsmPrinter.cpp

if (name.empty()) {
     valueIDs[value] = nextValueID++;
     return;
   }
 
   valueIDs[value] = NameSentinel;
   valueNames[value] = uniqueValueName(name);
 }
 
+// Returns true if 'c' is an allowable punctuation character: [$._-]
+// Returns false otherwise.
+static bool isPunct(char c) {
+  return c == '$' || c == '.' || c == '_' || c == '-';
+}
+  
 StringRef SSANameState::uniqueValueName(StringRef name) {
+  assert(!name.empty() && "Shouldn't have an empty name here");
+  
+  // Check to see if this name is valid.  If it starts with a digit, then it
+  // could conflict with the autogenerated numeric ID's (we unique them in a
+  // different map), so add an underscore prefix to avoid problems.
+  if (isdigit(name[0])) {
+    SmallString<16> tmpName("_");
+    tmpName += name;
+    return uniqueValueName(tmpName);
+  }
+  
+  // Check to see if the name consists of all-valid identifiers.  If not, we
+  // need to escape them.
+  for (auto ch : name) {
+    if (isalpha(ch) || isPunct(ch) || isdigit(ch))
+      continue;
+    
+    SmallString<16> tmpName;
+    for (auto ch : name) {
+      if (isalpha(ch) || isPunct(ch) || isdigit(ch))
+        tmpName += ch;
+      else if (ch == ' ')
+        tmpName += '_';
+      else {
+        tmpName += llvm::utohexstr((unsigned char)ch);
+      }
+    }
+    return uniqueValueName(tmpName);
+  }
+  
   // Check to see if this name is already unique.
   if (!usedNames.count(name)) {
     name = name.copy(usedNameAllocator);
   } else {
     // Otherwise, we had a conflict - probe until we find a unique name. This
     // is guaranteed to terminate (and usually in a single iteration) because it
     // generates new names by incrementing nextConflictID.
     SmallString<64> probeName(name);

mlir/test/IR/pretty-region-args.mlir

 // RUN: mlir-opt %s | FileCheck %s
 
 // CHECK-LABEL: func @custom_region_names
 func @custom_region_names() -> () {
   "test.polyfor"() ( {
   ^bb0(%arg0: index, %arg1: index, %arg2: index):
     "foo"() : () -> ()
   }) { arg_names = ["i", "j", "k"] } : () -> ()
   // CHECK: test.polyfor
   // CHECK-NEXT: ^bb{{.*}}(%i: index, %j: index, %k: index):
   return
 }
+
+// CHECK-LABEL: func @weird_names
+// Make sure the asmprinter handles weird names correctly.
+func @weird_names() -> () {
+  "test.polyfor"() ( {
+  ^bb0(%arg0: i32, %arg1: i32, %arg2: index):
+    "foo"() : () -> i32
+  }) { arg_names = ["a .^x", "0"] } : () -> ()
+  // CHECK: test.polyfor
+  // CHECK-NEXT: ^bb{{.*}}(%a_.5Ex: i32, %_0: i32, %arg0: index):
+  // CHECK-NEXT: %0 = "foo"()
+  return
+}
+