[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-460
   this->NumOutputs = NumOutputs;
   this->NumInputs = NumInputs;
   this->NumClobbers = NumClobbers;
+  this->NumLabels = NumLabels;
----------------
Please assert that you don't have both outputs and labels here. (If you did, you would assign them the same slots within `Exprs`.)

You also seem to be missing `Sema` enforcement of the rule that you cannot have both outputs and labels. (If you want to actually support that as an intentional extension to the GCC functionality, you should change the implementation of `GCCAsmStmt` to use different slots for outputs and labels, add some tests, and add a `-Wgcc-compat` warning for that case. Seems better to just add an error for it for now.)


================
Comment at: lib/Analysis/CFG.cpp:1445-1458
+      LabelMapTy::iterator LI = LabelMap.find(G->getLabel());;
+      // If there is no target for the goto, then we are looking at an
+      // incomplete AST.  Handle this by not registering a successor.
+      if (LI == LabelMap.end()) continue;
 
-    JumpTarget JT = LI->second;
-    prependAutomaticObjLifetimeWithTerminator(B, I->scopePosition,
-                                              JT.scopePosition);
-    prependAutomaticObjDtorsWithTerminator(B, I->scopePosition,
-                                           JT.scopePosition);
-    const VarDecl *VD = prependAutomaticObjScopeEndWithTerminator(
-        B, I->scopePosition, JT.scopePosition);
-    appendScopeBegin(JT.block, VD, G);
-    addSuccessor(B, JT.block);
+      JumpTarget JT = LI->second;
+      prependAutomaticObjLifetimeWithTerminator(B, I->scopePosition,
----------------
Please factor out this duplicated code into a local lambda rather than copy-pasting it.


================
Comment at: lib/Analysis/CFG.cpp:1461
+    if (auto *G = dyn_cast<GCCAsmStmt>(B->getTerminator())) {
+      if (G->isAsmGoto()) {
+        for (auto *E : G->labels()) {
----------------
This check is redundant; there will be no labels if it's not an `asm goto` so you can just perform the below loop unconditionally.


================
Comment at: lib/Analysis/CFG.cpp:3139-3141
+      // We will need to backpatch this block later.
+      BackpatchBlocks.push_back(JumpSource(Block, ScopePos));
+      return Block;
----------------
Do we not need the handling for the other labels if we hit this condition for one label?


================
Comment at: lib/Sema/SemaStmtAsm.cpp:656
+  // Check for duplicate asm operand name between input, output and label lists.
+  typedef std::pair <StringRef , Expr *> MyItemType;
+  SmallVector<MyItemType, 4> MyList;
----------------
No space after `pair`, please.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:659-662
+  for (unsigned i = 0, e = NumOutputs + NumInputs + NumLabels; i != e; ++i)
+    if (NS->getIdentifier(i) && !NS->getIdentifier(i)->getName().empty())
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+                                         NS->getOperandExpr(i)));
----------------
Looping over all three lists of expressions here is effectively hardcoding an implementation detail of `GCCAsmStmt` into this code, violating its encapsulation. Instead, please write three loops (iterating over the inputs, outputs, and labels), and remove `getOperandExpr` entirely.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:660
+  for (unsigned i = 0, e = NumOutputs + NumInputs + NumLabels; i != e; ++i)
+    if (NS->getIdentifier(i) && !NS->getIdentifier(i)->getName().empty())
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
----------------
Checking whether the name is empty is redundant: we should never create an `IdentifierInfo` representing an empty string.


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

https://reviews.llvm.org/D56571