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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: lib/Analysis/CFG.cpp:3137
+
+  Block = createBlock(false);
+  Block->setTerminator(G);
----------------
Passing add_successor=false seems suspect; this could probably use more test coverage.


================
Comment at: lib/CodeGen/CGStmt.cpp:2182
+    }
+  }
+
----------------
jyu2 wrote:
> nickdesaulniers wrote:
> > If this new block was moved closer to the new one on L2227, I assume they could be combined and possibly `IsGCCAsmGoto` be removed?  The code currently in between doesn't appear at first site to depend on info from this block, though maybe I may be missing it.
> The labels need be processed before Clobbers
Before the clobbers?  I don't see the relationship; the clobbers are just a string.

Granted, it does need to be before we compute the type, which is shared code.


================
Comment at: lib/CodeGen/CGStmt.cpp:1884
+template <typename T>
+void CodeGenFunction::UpdateCallInst(
+    T &Result, bool HasSideEffect, bool ReadOnly, bool ReadNone,
----------------
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.


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


================
Comment at: test/Parser/asm-goto.c:1
+// RUN: %clang_cc1 %s
+
----------------
What is this testing?


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

https://reviews.llvm.org/D56571