[TableGen] Fix non-standard escape warnings for braces in InstAlias

[c-rhodes]

Summary

TableGen interprets braces ('{}') in the asm string of instruction aliases as
variants but when defining aliases with literal braces they have to be escaped
to prevent them being removed.

Braces are escaped with '\\', for example:

def FooBraces : InstAlias<"foo \\{$imm\\}", (foo IntOperand:$imm)>;

Although when TableGen is emitting the assembly writer (-gen-asm-writer)
the AsmString that gets emitted is:

AsmString = "foo \{$\x01\}";

In c/c++ braces don't need to be escaped which causes compilation
warnings:

warning: use of non-standard escape character '\{' [-Wpedantic]

This patch fixes the issue by unescaping the flattened alias asm string
in the asm writer, by replacing '\{\}' with '{}'.

[hfinkel]

llvm/utils/TableGen/AsmWriterEmitter.cpp

827

Is this unescaping the right thing to do in general, or do we really just want to do it for the braces? What if there were an escaped quote in the string?

[c-rhodes]

llvm/utils/TableGen/AsmWriterEmitter.cpp

827

We probably only want to do this for braces. I wasn't too sure about adding this UnescapeString incase we get any undesired behaviour but the tests didn't raise any issues. Maybe it would be better if I create a new function UnescapeAliasString to handle this particular case?

[hfinkel]

llvm/utils/TableGen/AsmWriterEmitter.cpp

827

I recommend adding some test cases with some other escapes (quotes, new lines) and, if they don't work correctly, add a UnescapeAliasString. If everything does work correctly, then leave it as-is + the extra tests.

[c-rhodes]

llvm/utils/TableGen/AsmWriterEmitter.cpp

827

I tried some other escapes but don't really know what to make of it as the current behaviour doesn't look right.

I defined an alias containing an escaped quote as you suggested:

def FooQuote : InstAlias<"foo \"$imm\"", (foo IntOperand:$imm)>;

and generated the assembly writer with:

$ ./bin/llvm-tblgen -gen-asm-writer -I ../llvm/include ../llvm/test/TableGen/AliasAsmString.td -o AsmStringWriter.inc

the asm string that gets emitted is invalid c++:

static const char AsmStrings[] =
  /* 0 */ "foo "$\x01"\0"
;

but the behaviour is the same without this patch. I'm tempted to add UnescapeAliasString even though this patch isn't changing escapes in the alias string because I don't know what the correct behaviour is and I don't want to introduce tests that validate potentially incorrect behaviour. Any thoughts?

[c-rhodes]

Changes:

• Move unescaping of braces in InstAlias asm string from UnescapeString to UnecapeAliasString .

[hfinkel]

LGTM

llvm/utils/TableGen/AsmWriterEmitter.cpp

827

Fair enough; the non-compilable code certainly isn't useful, but maybe we've never really supported things in the strings that would need escaping when output anyway. If so, this is a special case distinct from those.

llvm/test/TableGen/AliasAsmString.td

+// RUN: llvm-tblgen -gen-asm-writer -I %p/../../include %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+
+def ArchInstrInfo : InstrInfo { }
+
+def Arch : Target {
+  let InstructionSet = ArchInstrInfo;
+}
+
+def Reg : Register<"reg">;
+
+def RegClass : RegisterClass<"foo", [i32], 0, (add Reg)>;
+
+def IntOperand: Operand<i32>;
+
+def foo : Instruction {
+  let Size = 2;
+  let OutOperandList = (outs);
+  let InOperandList = (ins IntOperand:$imm);
+  let AsmString = "foo $imm";
+  let Namespace = "Arch";
+}
+
+def FooBraces : InstAlias<"foo \\{$imm\\}", (foo IntOperand:$imm)>;
+
+// CHECK: static const char AsmStrings[] =
+// CHECK-NEXT: /* 0 */ "foo {$\x01}\0"

llvm/utils/TableGen/AsmWriterEmitter.cpp

if (Str[i] == '\\' && i != Str.size()-1) {
       case '\\': Str[i] = '\\'; break;
       }
       // Nuke the second character.
       Str.erase(Str.begin()+i+1);
     }
   }
 }
 
+/// UnescapeAliasString - Supports literal braces in InstAlias asm string which
+/// are escaped with '\\' to avoid being interpreted as variants. Braces must
+/// be unescaped before c++ code is generated as (e.g.):
+///
+///   AsmString = "foo \{$\x01\}";
+///
+/// causes non-standard escape character warnings.
+static void UnescapeAliasString(std::string &Str) {
+  for (unsigned i = 0; i != Str.size(); ++i) {
+    if (Str[i] == '\\' && i != Str.size()-1) {
+      switch (Str[i+1]) {
+      default: continue;  // Don't execute the code after the switch.
+      case '{': Str[i] = '{'; break;
+      case '}': Str[i] = '}'; break;
+      }
+      // Nuke the second character.
+      Str.erase(Str.begin()+i+1);
+    }
+  }
+}
+
 /// EmitPrintInstruction - Generate the code for the "printInstruction" method
 /// implementation. Destroys all instances of AsmWriterInst information, by
 /// clearing the Instructions vector.
 void AsmWriterEmitter::EmitPrintInstruction(raw_ostream &O) {
   Record *AsmWriter = Target.getAsmWriter();
   StringRef ClassName = AsmWriter->getValueAsString("AsmWriterClassName");
   bool PassSubtarget = AsmWriter->getValueAsInt("PassSubtarget");
 
for (auto &Alias : Aliases.second) {
       unsigned LastOpNo = CGA.ResultInstOperandIndex.size();
       std::string FlatInstAsmString =
          CodeGenInstruction::FlattenAsmStringVariants(CGA.ResultInst->AsmString,
                                                       Variant);
       unsigned NumResultOps = CountNumOperands(FlatInstAsmString, Variant);
 
       std::string FlatAliasAsmString =
           CodeGenInstruction::FlattenAsmStringVariants(CGA.AsmString, Variant);
+      UnescapeAliasString(FlatAliasAsmString);
 
       // Don't emit the alias if it has more operands than what it's aliasing.
       if (NumResultOps < CountNumOperands(FlatAliasAsmString, Variant))
         continue;
 
       StringRef Namespace = Target.getName();
       unsigned NumMIOps = 0;
       for (auto &ResultInstOpnd : CGA.ResultInst->Operands)