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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: lib/Analysis/CFG.cpp:1474
+          appendScopeBegin(JT.block, VD, G);
+          addSuccessor(B, JT.block);
+        }
----------------
Please don't copy-paste code.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:470
+    if (NS->isGCCAsmGoto() &&
+        Exprs[ConstraintIdx]->getStmtClass() == Stmt::AddrLabelExprClass)
+      break;
----------------
jyu2 wrote:
> efriedma wrote:
> > jyu2 wrote:
> > > efriedma wrote:
> > > > This looks suspicious; an AddrLabelExpr could be an input or output, e.g. `"r"(&&foo)`.
> > > Syntax for asm goto:
> > >  Syntax:
> > >    asm [volatile] goto ( AssemblerTemplate
> > >                        :
> > >                        : InputOperands
> > >                        : Clobbers
> > >                        : GotoLabels)
> > > 
> > >  Only input is allowed.  Output is not allowed
> > > 
> > That doesn't really address my point here... ignore the "or output" part of the comment.
> Sorry did not realize that.  Thank you so much for catching that.  Need to add other condition "ConstraintIdx > NS->getNumInputs() - 1", change to :
> 
> if (NS->isGCCAsmGoto() && ConstraintIdx > NS->getNumInputs() - 1 &&
>         Exprs[ConstraintIdx]->getStmtClass() == Stmt::AddrLabelExprClass)
>       break;
> 
> Is this ok with you?  Thanks
That's the right idea. But I still see a few issues at that point:

1. The AddrLabelExprClass check is redundant.
2. "NS->getNumInputs() - 1" can overflow; probably should use "ConstraintIdx >= NS->getNumInputs()".
3. "break" exits the loop completely (so it skips validating all constraints written after the label).
4. The code needs to verify that the user correctly specified the "l" constraint modifier.


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

https://reviews.llvm.org/D56571