[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: include/clang/AST/Stmt.h:1008
+             (*iterator_adaptor_base::I)->getStmtClass() <= lastExprConstant);
+      return *reinterpret_cast<T **>(iterator_adaptor_base::I);
     }
----------------
Ugh. This cast violates strict aliasing, and even in the absence of strict aliasing won't work unless the `Stmt` base class is at offset 0 in `T`. The preceding assert is also wrong, as it's asserting that `*I` is an `Expr`, not that it's a `T`.

After r352925, you can use `CastIterator<AddrLabelExpr>` instead; that should substantially reduce the churn in this patch.


================
Comment at: include/clang/AST/Stmt.h:2842
+
+  bool isGCCAsmGoto() const {
+    return NumLabels > 0;
----------------
This is already inside a class `GCCAsmStmt`; the `GCC` here seems unnecessary.


================
Comment at: include/clang/AST/Stmt.h:2860
+  StringRef getLabelName(unsigned i) const;
+  Expr *getExpr(unsigned i) const;
+  using labels_iterator = ExprIterator<AddrLabelExpr>;
----------------
Please give this a better name; `getExpr` is pretty meaningless.


================
Comment at: lib/Analysis/CFG.cpp:1466
+          LabelMapTy::iterator LI = LabelMap.find(E->getLabel());
+          if (LI == LabelMap.end()) continue;
+          JT = LI->second;
----------------
If this happens for every iteration of the loop, `VD` is used uninitialized after the loop.


================
Comment at: lib/Analysis/CFG.cpp:1472
+                                                 JT.scopePosition);
+          VD = prependAutomaticObjScopeEndWithTerminator(B, I->scopePosition,
+                                                         JT.scopePosition);
----------------
It is extremely suspicious to be overwriting the value of `VD` on each iteration of the loop like this. If you are assuming that the same value will be returned every time, please include an `assert` of that.


================
Comment at: lib/Analysis/CFG.cpp:2126
+      if (cast<GCCAsmStmt>(S)->isGCCAsmGoto())
+        return VisitGCCAsmStmt(cast<GCCAsmStmt>(S));
+      return VisitStmt(S, asc);
----------------
This is not an appropriate function name for a function that is only called on `asm goto`. Given the convention here is to match the `Stmt` class hierarchy with the `Visit` functions, the branch on `asm goto` should be in the callee.


================
Comment at: lib/CodeGen/CGStmt.cpp:1884
+template <typename T>
+void CodeGenFunction::UpdateCallInst(
+    T &Result, bool HasSideEffect, bool ReadOnly, bool ReadNone,
----------------
This is not a reasonable function name for a function that is specific to `AsmStmt`s. Please also make this a (static) non-member function, since it cannot be called outside this source file.


================
Comment at: lib/CodeGen/CGStmt.cpp:2238-2245
+    UpdateCallInst<llvm::CallBrInst>(*Result, HasSideEffect, ReadOnly,
+                                     ReadNone, S, ResultRegTypes, RegResults);
+    EmitBlock(Fallthrough);
   } else {
-    for (unsigned i = 0, e = ResultRegTypes.size(); i != e; ++i) {
-      llvm::Value *Tmp = Builder.CreateExtractValue(Result, i, "asmresult");
-      RegResults.push_back(Tmp);
-    }
+    llvm::CallInst *Result =
+        Builder.CreateCall(IA, Args, getBundlesForFunclet(IA));
+    UpdateCallInst<llvm::CallInst>(*Result, HasSideEffect, ReadOnly, ReadNone,
----------------
No need to explicitly specify the template argument here; it's deducible.


================
Comment at: lib/Parse/ParseStmtAsm.cpp:833-837
+    if (Tok.isNot(tok::identifier)) {
+      Diag(Tok, diag::err_expected) << tok::identifier;
+      SkipUntil(tok::r_paren, StopAtSemi);
+      return StmtError();
+    }
----------------
This should be inside the loop below.


================
Comment at: lib/Parse/ParseStmtAsm.cpp:839
+    if (Tok.isNot(tok::r_paren)) {
+      while (1) {
+        LabelDecl *LD = Actions.LookupOrCreateLabel(Tok.getIdentifierInfo(),
----------------
`while (true)`


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

https://reviews.llvm.org/D56571