[PATCH] D136497: [Clang] support for outputs along indirect edges of asm goto

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

It might be easier to see the main changes here if you submit the (very nice) refactoring of `EmitAsmStores` first.



================
Comment at: clang/lib/CodeGen/CGStmt.cpp:2358
+    // the expression, do the conversion.
+    if (ResultRegTypes[i] != ResultTruncRegTypes[i]) {
+
----------------
s/ResultTruncRegTypes[i]/TruncTy/


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:2362
+      // a pointer.
+      if (TruncTy->isFloatingPointTy())
+        Tmp = Builder.CreateFPTrunc(Tmp, TruncTy);
----------------
This looks like a direct copy from below (which is fine). I'm a bit iffy on whether this covers *all* of the value types that could come through here...


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:2850
 
   assert(RegResults.size() == ResultRegTypes.size());
   assert(RegResults.size() == ResultTruncRegTypes.size());
----------------
Should these asserts (or some of them) be moved into `EmitAsmStores()`?


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:2862
+
+  if (IsGCCAsmGoto && CBRRegResults.size()) {
+    for (unsigned i = 0, e = CBR->getNumIndirectDests(); i != e; ++i) {
----------------
nit: Could you add a comment explaining that we're calling `EmitAsmStores` for asm goto's indirect branches? I was slightly confused about why we were calling `EmitAsmStores` more than once. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136497