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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
