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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 marked 3 inline comments as done.
jyu2 added inline comments.


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


================
Comment at: lib/Sema/SemaStmtAsm.cpp:470
+    if (NS->isGCCAsmGoto() &&
+        Exprs[ConstraintIdx]->getStmtClass() == Stmt::AddrLabelExprClass)
+      break;
----------------
efriedma wrote:
> 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.
Sorry not done yet.  


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

https://reviews.llvm.org/D56571