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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 commandeered this revision.
jyu2 edited reviewers, added: craig.topper; removed: jyu2.
jyu2 added inline comments.


================
Comment at: lib/Analysis/CFG.cpp:2126
+      if (cast<GCCAsmStmt>(S)->isGCCAsmGoto())
+        return VisitGCCAsmStmt(cast<GCCAsmStmt>(S));
+      return VisitStmt(S, asc);
----------------
rsmith wrote:
> 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.
Changed.


================
Comment at: lib/CodeGen/CGStmt.cpp:1884
+template <typename T>
+void CodeGenFunction::UpdateCallInst(
+    T &Result, bool HasSideEffect, bool ReadOnly, bool ReadNone,
----------------
efriedma wrote:
> rsmith wrote:
> > 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.
> Does this really need to be a template function?  I think all the functions you're calling on "Result" are members of CallBase.
How about UpdateAsmCallInst?


================
Comment at: lib/CodeGen/CGStmt.cpp:1884
+template <typename T>
+void CodeGenFunction::UpdateCallInst(
+    T &Result, bool HasSideEffect, bool ReadOnly, bool ReadNone,
----------------
jyu2 wrote:
> efriedma wrote:
> > rsmith wrote:
> > > 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.
> > Does this really need to be a template function?  I think all the functions you're calling on "Result" are members of CallBase.
> How about UpdateAsmCallInst?
Yes. I am using CallBase instead template.


================
Comment at: lib/CodeGen/CGStmt.cpp:2188
+                                     std::to_string(NumGccAsmGotoStmts));
+      NumGccAsmGotoStmts++;
+    }
----------------
efriedma wrote:
> The NumGccAsmGotoStmts variable is pointless; if names are enabled, createBasicBlock will automatically number all the blocks named "normal".
Okay changed.  Thanks.


================
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,
----------------
rsmith wrote:
> No need to explicitly specify the template argument here; it's deducible.
Thanks.  Changed.


================
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();
+    }
----------------
rsmith wrote:
> This should be inside the loop below.
Yes.  Change.


================
Comment at: test/Parser/asm-goto.c:1
+// RUN: %clang_cc1 %s
+
----------------
efriedma wrote:
> What is this testing?
Just testing asm goto can parser those input operands and labels.  Not error checking.  

Error checking is in Sema/asm-goto


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

https://reviews.llvm.org/D56571