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

[Jennifer Yu via Phabricator via cfe-commits]

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


================
Comment at: lib/AST/Stmt.cpp:457-460
   this->NumOutputs = NumOutputs;
   this->NumInputs = NumInputs;
   this->NumClobbers = NumClobbers;
+  this->NumLabels = NumLabels;
----------------
rsmith wrote:
> jyu2 wrote:
> > rsmith wrote:
> > > Please assert that you don't have both outputs and labels here. (If you did, you would assign them the same slots within `Exprs`.)
> > > 
> > > You also seem to be missing `Sema` enforcement of the rule that you cannot have both outputs and labels. (If you want to actually support that as an intentional extension to the GCC functionality, you should change the implementation of `GCCAsmStmt` to use different slots for outputs and labels, add some tests, and add a `-Wgcc-compat` warning for that case. Seems better to just add an error for it for now.)
> > This is enforcement during the parer.  
> > 
> > for example:
> > a.c:12:23: error: expected ':'
> > asm goto ("decl %0;" :"a": "m"(cond) : "memory" );
> > 
> > Do you think this is enough for now?
> > Thanks.
> > Jennifer
> Thanks, I see it now. Please still add the assert here.
> 
> I'd also like a custom diagnostic for that parse error; it'd seem easy and useful to add an "asm goto cannot have output constraints" error.
Okay added.


================
Comment at: lib/CodeGen/CGStmt.cpp:2236
+    llvm::CallBrInst *Result =
+        Builder.CreateCallBr(FTy, IA, Fallthrough, Transfer, Args);
+    UpdateAsmCallInst(cast<llvm::CallBase>(*Result), HasSideEffect, ReadOnly,
----------------
craig.topper wrote:
> I don't think we need to pass FTy here now. I made that change before @jyknight added FunctionCallee that should be able to find the type from the InlineAsm. The CallBr IRBuilder code should have a FunctionCallee Create function as of yesterday.
Removed.


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

https://reviews.llvm.org/D56571