[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/Stmt.cpp:457-458
+  this->NumLabels = NumLabels;
+  assert((NumLabels != 0 && NumOutputs == 0) ||
+         (NumOutputs != 0 && NumLabels == 0));
 
----------------
This can be simplified a little:
```
assert(!(NumOutputs && NumLabels) && "asm goto cannot have outputs");
```


================
Comment at: lib/AST/Stmt.cpp:626
+      if (CheckLabels) {
+        if (N + 1 <= NumOperands - getNumLabels()) {
+          DiagOffs = CurPtr-StrStart-1;
----------------
`N < NumOperands - getNumLabels()`


================
Comment at: lib/AST/Stmt.cpp:628
+          DiagOffs = CurPtr-StrStart-1;
+          return diag::err_asm_invalid_operand_for_goto_labels;
+        }
----------------
I'm curious why we're checking this here, when all the other validation for the modifier letter happens in LLVM. Does this check need to be done differently from the others?


================
Comment at: lib/CodeGen/CGStmt.cpp:2186
+      }
+      StringRef Name = "normal";
+      Fallthrough = createBasicBlock(Name);
----------------
Nit: I think something like "asm.fallthrough" or "asm.cont" would be a more obvious name for the fall-through block.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:659-671
+    if (NS->getIdentifier(i))
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+                                         Exprs[i]));
+  for (unsigned i = NumOutputs, e = NumOutputs + NumInputs; i != e; ++i)
+    if (NS->getIdentifier(i))
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+                                         Exprs[i]));
----------------
This is still exposing / relying on an implementation detail of `GCCAsmStmt`. Please replace `getIdentifier` with `getOuptutIdentifier` / `getInputIdentifier` / `getLabelIdentifier`, adjust the indexing to match, and remove `GCCAsmStmt::getIdentifier`. Or alternatively, iterate over `Names` here rather than walking the parts of the `GCCAsmStmt`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56571/new/

https://reviews.llvm.org/D56571