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

Jennifer Yu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 5 19:46:02 PST 2019


jyu2 added inline comments.


================
Comment at: lib/AST/Stmt.cpp:457-458
+  this->NumLabels = NumLabels;
+  assert((NumLabels != 0 && NumOutputs == 0) ||
+         (NumOutputs != 0 && NumLabels == 0));
 
----------------
rsmith wrote:
> This can be simplified a little:
> ```
> assert(!(NumOutputs && NumLabels) && "asm goto cannot have outputs");
> ```
Thanks.  Yes, this is better than me.  Changed


================
Comment at: lib/AST/Stmt.cpp:628
+          DiagOffs = CurPtr-StrStart-1;
+          return diag::err_asm_invalid_operand_for_goto_labels;
+        }
----------------
rsmith wrote:
> I'm curious why we're checking this here, when all the other validation for the modifier letter happens in LLVM. Does this check need to be done differently from the others?
This is to verify following in spec:
=====
To reference a label in the assembler template, prefix it with ‘%l’ (lowercase ‘L’) followed by its (zero-based) position in GotoLabels plus the number of input operands. For example, if the asm has three inputs and references two labels, refer to the first label as ‘%l3’ and the second as ‘%l4’).

Was request from Eli.


================
Comment at: lib/CodeGen/CGStmt.cpp:2186
+      }
+      StringRef Name = "normal";
+      Fallthrough = createBasicBlock(Name);
----------------
rsmith wrote:
> Nit: I think something like "asm.fallthrough" or "asm.cont" would be a more obvious name for the fall-through block.
Changed.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:659-671
+    if (NS->getIdentifier(i))
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+                                         Exprs[i]));
+  for (unsigned i = NumOutputs, e = NumOutputs + NumInputs; i != e; ++i)
+    if (NS->getIdentifier(i))
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+                                         Exprs[i]));
----------------
rsmith wrote:
> This is still exposing / relying on an implementation detail of `GCCAsmStmt`. Please replace `getIdentifier` with `getOuptutIdentifier` / `getInputIdentifier` / `getLabelIdentifier`, adjust the indexing to match, and remove `GCCAsmStmt::getIdentifier`. Or alternatively, iterate over `Names` here rather than walking the parts of the `GCCAsmStmt`.
Changed.  Like this!!!


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

https://reviews.llvm.org/D56571





More information about the cfe-commits mailing list