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