[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG23c2a5ce33f0: Allow callbr to return non-void 
values (authored by void).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,160 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-LABEL: .Ltmp0: # Address of block that was removed by CodeGen
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  jmp .LBB1_4
+; CHECK-NEXT:  .LBB1_3: # %if.else
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  jne .Ltmp2
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_4:
+; CHECK-NEXT:  movl %esi, %eax
+; CHECK-NEXT:  addl %edi, %eax
+; CHECK-NEXT:  .Ltmp2:
+; CHECK-NEXT:  # %bb.5: # %return
+; CHECK-LABEL: .Ltmp1: # Address of block that was removed by CodeGen
+define i32 @test2(i32 %out1, i32 %out2) {
+entry:
+  %cmp = icmp slt i32 %out1, %out2
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:  ; preds = %entry
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "={si},={di},r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.else:  ; preds = %entry
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "={si},={di},r,r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i32 %out2, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.end:   ; preds = %if.else, %if.then
+  %.sink11 = phi { i32, i32 } [ %0, %if.then ], [ %1, %if.else ]
+  %asmresult3 = extractvalue { i32, i32 } %.sink11, 0
+  %asmresult4 = extractvalue { i32, i32 } %.sink11, 1
+  %add = add nsw i32 %asmresult4, %asmresult3
+  br label %return
+
+label_true:   ; preds = %if.else, %if.then
+  br label %return
+
+return:   ; preds = %if.then, %if.else, %label_true, %if.end
+  %retval.0 = phi i32 [ %add, %if.end ], [ -2, %label_true ], [ -1, %if.else ], [ -1, %if.then ]
+  ret i32 %retval.0
+}
+
+; CHECK-LABEL: test3:
+; CHECK:   # %bb.1: # %true
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  .short %esi
+; CHECK-NEXT:  .short %edi
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:137
+  /// List of indirect targets of the callbr of a basic block.
+  SmallPtrSet InlineAsmBrIndirectTargets;
+

nickdesaulniers wrote:
> It's likely the count here is 0, or maybe 1.  We don't see too often a large 
> list of labels here.
I'll make it 2 instead. :-)



Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:1076
+// to the copy so that everyone is happy.
+for (auto *Succ : BB->successors())
+  if (Succ != CopyBB && !CopyBB->isSuccessor(Succ))

nickdesaulniers wrote:
> Isn't `Fallthrough` from above one of the potential successors? Do we have to 
> skip it in the below conditional? What happens if we call `addSuccessor` with 
> the same `MBB` twice?
Yes. It's added to CopyBB above and the `!CopyBB->isSuccessor(Succ)` makes sure 
it's not re-added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Quentin Colombet via Phabricator via cfe-commits]

qcolombet added a comment.

> I've been concerned about the register live-ins too (I'm less concerned about 
> the successors issue). Is there documentation on the original decision to 
> disallow physical register live-ins for MBBs before register allocation? We 
> could then check to see if we're violating the original reasoning.

IIRC the regallocators don't rely on this. Actually, I think the live-ins sets 
are supposed to be correct only after regalloc (expect for the entry block, I 
don’t know for the landing pads.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.

I'm super excited to see this progress towards supporting 'asm goto' with 
results!  Great work!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Would you be okay with me submitting this and working on making INLINEASM_BR a 
non-terminator? I'd like to give this feature some bake time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D69868#1883687 , @jyknight wrote:

> Ugh, it's actually been that long, hasn't it...I'm really sorry about that. :(


No worries. Thanks for getting back to us!

> I've been actively spending time to look at this over the last couple weeks. 
> I haven't been able to convince myself that the weird-successors and having 
> allocatable registers across BBs here is not going to cause codegen issues 
> after optimization passes run on it. Unfortunately, despite spending time 
> looking into it, I also wasn't able to convince myself that this *IS* broken. 
> Maybe someone else can chime in here and either assuage or confirm my worries?

I've been concerned about the register live-ins too (I'm less concerned about 
the successors issue). Is there documentation on the original decision to 
disallow physical register live-ins for MBBs before register allocation? We 
could then check to see if we're violating the original reasoning.

> I also see some other minor issues, which I need to write up, but I'd been 
> blocking writing up a comment for that behind the larger question.
> 
> Anyways, while looking at this I started thinking it might actually be better 
> to actually have INLINEASM_BR (both with or without outputs) _not_ be a 
> terminator MachineInstr. (remaining a terminator in IR form, however). This 
> would then be similar to how "invoke" works -- at the MachineInstr level, the 
> call is not a terminator, even though it can jump out to the EH successors. 
> And, the return-value handling remains in the same basic block as the call. I 
> tried making that change, but doing it correctly has other impacts -- much of 
> the code which currently special-cases isEHPad() needs to be updated (Which 
> does mean I now feel like I have a good handle on the problems the _previous_ 
> version of this code, which didn't split the block, had.). 
> MachineBasicBlock::updateTerminator is a good example of the problematic 
> cases -- it trawls the successor list to look for the "correct" successor, 
> filtering out isEHPad successors. But unlike EH Pads, indirect targets of a 
> INLINEASM_BR are not used only for that purpose, so can't be so quickly 
> distinguished in the successors list.

I had a very similar idea. There are some people *cough*Chris*cough* who insist 
that `INLINEASM_BR` makes sense as a terminator. I don't deny that it's very 
tempting to make it one, but because of this instruction's behavior it doesn't 
*act* like a normal terminator (explicitly branching to some destination).

As for `isEHPad`, do you think it would make sense to have a generic 
`isIndirectTarget` predicate, which we could then use everywhere and it would 
automagically filter out blocks that aren't interesting?

> So, I started updating some of that code to not have that assumption. While 
> doing so, I noticed that fixing this would also allow analyzeBranch to ignore 
> the inlineasm branch instructions -- and remain correct -- which actually 
> would allow llvm to do better block placement of the inlineasm_br blocks, 
> because it enables it to move the normal successor jump/fallthrough. So, I 
> think that would actually be a good idea to make that change.

Now that I know I'm not crazy for wanting to make INLINEASM_BR a non-terminator 
(I'm assuming you're not crazy at least :-), I'll give it a go. Feel free to 
send me patches if you have them.

> But whether it'd be _necessary_ to make such a change (or other changes) for 
> this patch, or just a nice thing to fix, I'm still just unsure about.

Given your concerns over the patch in its current form, let's at least try the 
non-terminator path. If we can make it work, then your concerns will be 
assuaged (as would mine).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ugh, it's actually been that long, hasn't it...I'm really sorry about that. :(

I've been actively spending time to look at this over the last couple weeks. I 
haven't been able to convince myself that the weird-successors and having 
allocatable registers across BBs here is not going to cause codegen issues 
after optimization passes run on it. Unfortunately, despite spending time 
looking into it, I also wasn't able to convince myself that this *IS* broken. 
Maybe someone else can chime in here and either assuage or confirm my worries?

I also see some other minor issues, which I need to write up, but I'd been 
blocking writing up a comment for that behind the larger question.

Anyways, while looking at this I started thinking it might actually be better 
to actually have INLINEASM_BR (both with or without outputs) _not_ be a 
terminator MachineInstr. (remaining a terminator in IR form, however). This 
would then be similar to how "invoke" works -- at the MachineInstr level, the 
call is not a terminator, even though it can jump out to the EH successors. 
And, the return-value handling remains in the same basic block as the call. I 
tried making that change, but doing it correctly has other impacts -- much of 
the code which currently special-cases isEHPad() needs to be updated (Which 
does mean I now feel like I have a good handle on the problems the _previous_ 
version of this code, which didn't split the block, had.). 
MachineBasicBlock::updateTerminator is a good example of the problematic cases 
-- it trawls the successor list to look for the "correct" successor, filtering 
out isEHPad successors. But unlike EH Pads, indirect targets of a INLINEASM_BR 
are not used only for that purpose, so can't be so quickly distinguished in the 
successors list.

So, I started updating some of that code to not have that assumption. While 
doing so, I noticed that fixing this would also allow analyzeBranch to ignore 
the inlineasm branch instructions -- and remain correct -- which actually would 
allow llvm to do better block placement of the inlineasm_br blocks, because it 
enables it to move the normal successor jump/fallthrough. So, I think that 
would actually be a good idea to make that change.

But whether it'd be _necessary_ to make such a change (or other changes) for 
this patch, or just a nice thing to fix, I'm still just unsure about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

This code has now been tested on a running Linux kernel making use of the 
feature.

I still would like @jyknight to clarify his comments, consider explicitly 
requesting changes to this CL, or consider resigning as reviewer.




Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:137
+  /// List of indirect targets of the callbr of a basic block.
+  SmallPtrSet InlineAsmBrIndirectTargets;
+

It's likely the count here is 0, or maybe 1.  We don't see too often a large 
list of labels here.



Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701
+  } else if (MBB->succ_size() == LandingPadSuccs.size() ||
+ MBB->succ_size() == IndirectPadSuccs.size()) {
 // It's possible that the block legitimately ends with a noreturn

void wrote:
> void wrote:
> > jyknight wrote:
> > > void wrote:
> > > > jyknight wrote:
> > > > > This isn't correct.
> > > > > 
> > > > > This line here, is looking at a block which doesn't end in a jump to 
> > > > > a successor. So, it's trying to verify that the successor list makes 
> > > > > sense in that context.
> > > > > 
> > > > > The unstated assumption in the code is that the only successors will 
> > > > > be landing pads. Instead of actually checking each one, instead it 
> > > > > just checks that the count is the number of landing pads, with the 
> > > > > assumption that all the successors should be landing pads, and that 
> > > > > all the landing pads should be successors.
> > > > > 
> > > > > The next clause is then checking for the case where there's a 
> > > > > fallthrough to the next block. In that case, the successors should've 
> > > > > been all the landing pads, and the single fallthrough block.
> > > > > 
> > > > > Adding similar code to check for the number of callbr targets doesn't 
> > > > > really make sense. It's certainly not the case that all callbr 
> > > > > targets are targets of all 
> > > > > callbr instructions. And even if it was, this still wouldn't be 
> > > > > counting things correctly.
> > > > > 
> > > > > 
> > > > > However -- I think i'd expect analyzeBranch to error out (returning 
> > > > > true) when confronted by a callbr instruction, because it cannot 
> > > > > actually tell what's going on there. If that were the case, nothing 
> > > > > in this block should even be invoked. But I guess that's probably not 
> > > > > happening, due to the terminator being followed by non-terminators.
> > > > > 
> > > > > That seems likely to be a problem that needs to be fixed. (And if 
> > > > > that is fixed, I think the changes here aren't needed anymore)
> > > > > 
> > > > > 
> > > > Your comment is very confusing. Could you please give an example of 
> > > > where this fails?
> > > Sorry about that, I should've delimited the parts of that message 
> > > better...
> > > Basically:
> > > - Paragraphs 2-4 are describing why the code before this patch appears to 
> > > be correct for landing pad, even though it's taking some shortcuts and 
> > > making some non-obvious assumptions.
> > > - Paragraph 5 ("Adding similar code"...) is why it's not correct for 
> > > callbr.
> > > - Paragraph 6-7 are how I'd suggest to resolve it.
> > > 
> > > 
> > > I believe the code as of your patch will fail validation if you have a 
> > > callbr instruction which has a normal-successor block which is an 
> > > indirect target of a *different* callbr in the function.
> > > 
> > > I believe it'll also fail if you have any landing-pad successors, since 
> > > those aren't being added to the count of expected successors, but rather 
> > > checked separately.
> > > 
> > > But more seriously than these potential verifier failures, I expect that 
> > > analyzeBranch returning wrong answers (in that it may report that a block 
> > > unconditionally-jumps to a successor, while it really has both a callbr 
> > > and jump, separated by the non-terminator copies) will cause 
> > > miscompilation. I'm not sure exactly how that will exhibit, but I'm 
> > > pretty sure it's not going to be good.
> > > 
> > > And, if analyzeBranch properly said "no idea" when confronted by callbr 
> > > control flow, then this code in the verifier wouldn't be reached.
> > I didn't need a delineation of the parts of the comment. I needed a clearer 
> > description of what your concern is, and to give an example of code that 
> > fails here.
> > 
> > This bit of code is simply saying that if the block containing the 
> > `INLINEASM_BR` doesn't end with a `BR` instruction, then the number of its 
> > successors should be equal to the number of indirect successors. This is 
> > correct, as it's not valid to have a duplicate label used in a `callbr` 
> > instruction:
> > 
> > ```
> > $ llc -o /dev/null x.ll
> > Duplicate callbr destination!
> >   %3 = callbr i32 asm sideeffect "testl $0, 

[PATCH] D69868: Allow "callbr" to return non-void values

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D69868#1881985 , @void wrote:

> It's been almost a month since the last comments on this review. If you need 
> more time, please comment here. Otherwise, I will submit this with the 
> current approvals by the end of the week.


Explicit ping @nickdesaulniers, who blocked this, and @jyknight and @rnk, both 
explicitly requested by @MaskRay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

It's been almost a month since the last comments on this review. If you need 
more time, please comment here. Otherwise, I will submit this with the current 
approvals by the end of the week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 244840.
void added a comment.

Use the BB when creating the MBB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,160 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-LABEL: .Ltmp0: # Address of block that was removed by CodeGen
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  jmp .LBB1_4
+; CHECK-NEXT:  .LBB1_3: # %if.else
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  jne .Ltmp2
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_4:
+; CHECK-NEXT:  movl %esi, %eax
+; CHECK-NEXT:  addl %edi, %eax
+; CHECK-NEXT:  .Ltmp2:
+; CHECK-NEXT:  # %bb.5: # %return
+; CHECK-LABEL: .Ltmp1: # Address of block that was removed by CodeGen
+define i32 @test2(i32 %out1, i32 %out2) {
+entry:
+  %cmp = icmp slt i32 %out1, %out2
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:  ; preds = %entry
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "={si},={di},r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.else:  ; preds = %entry
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "={si},={di},r,r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i32 %out2, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.end:   ; preds = %if.else, %if.then
+  %.sink11 = phi { i32, i32 } [ %0, %if.then ], [ %1, %if.else ]
+  %asmresult3 = extractvalue { i32, i32 } %.sink11, 0
+  %asmresult4 = extractvalue { i32, i32 } %.sink11, 1
+  %add = add nsw i32 %asmresult4, %asmresult3
+  br label %return
+
+label_true:   ; preds = %if.else, %if.then
+  br label %return
+
+return:   ; preds = %if.then, %if.else, %label_true, %if.end
+  %retval.0 = phi i32 [ %add, %if.end ], [ -2, %label_true ], [ -1, %if.else ], [ -1, %if.then ]
+  ret i32 %retval.0
+}
+
+; CHECK-LABEL: test3:
+; CHECK:   # %bb.1: # %true
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  .short %esi
+; CHECK-NEXT:  .short %edi
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB2_2:
+; CHECK-NEXT:  movl %edi, %eax
+; CHECK-NEXT:  jmp 

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Another friendly ping. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 242752.
void added a comment.

Use iterator for machine operands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,160 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-LABEL: .Ltmp0: # Address of block that was removed by CodeGen
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  jmp .LBB1_4
+; CHECK-NEXT:  .LBB1_3: # %if.else
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  jne .Ltmp2
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_4:
+; CHECK-NEXT:  movl %esi, %eax
+; CHECK-NEXT:  addl %edi, %eax
+; CHECK-NEXT:  .Ltmp2:
+; CHECK-NEXT:  # %bb.5: # %return
+; CHECK-LABEL: .Ltmp1: # Address of block that was removed by CodeGen
+define i32 @test2(i32 %out1, i32 %out2) {
+entry:
+  %cmp = icmp slt i32 %out1, %out2
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:  ; preds = %entry
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "={si},={di},r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.else:  ; preds = %entry
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "={si},={di},r,r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i32 %out2, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.end:   ; preds = %if.else, %if.then
+  %.sink11 = phi { i32, i32 } [ %0, %if.then ], [ %1, %if.else ]
+  %asmresult3 = extractvalue { i32, i32 } %.sink11, 0
+  %asmresult4 = extractvalue { i32, i32 } %.sink11, 1
+  %add = add nsw i32 %asmresult4, %asmresult3
+  br label %return
+
+label_true:   ; preds = %if.else, %if.then
+  br label %return
+
+return:   ; preds = %if.then, %if.else, %label_true, %if.end
+  %retval.0 = phi i32 [ %add, %if.end ], [ -2, %label_true ], [ -1, %if.else ], [ -1, %if.then ]
+  ret i32 %retval.0
+}
+
+; CHECK-LABEL: test3:
+; CHECK:   # %bb.1: # %true
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  .short %esi
+; CHECK-NEXT:  .short %edi
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB2_2:
+; CHECK-NEXT:  movl %edi, %eax
+; CHECK-NEXT:  jmp 

[PATCH] D69868: Allow "callbr" to return non-void values

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.

I think this is fine, but want to hear from @jyknight and @rnk.




Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:1039
+  auto  = BB->back();
+  for (unsigned i = 0; i < Last.getNumOperands(); ++i) {
+MachineOperand  = Last.getOperand(i);

`for (const MachineOperand  : Last.operands()) {`




Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:1062
+for (auto  : *CopyBB) {
+  for (unsigned i = 0; i < MI.getNumOperands(); ++i) {
+const MachineOperand  = MI.getOperand(i);

`for (const MachineOperand  : MI.operands()) {`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

My apologies for being a pest, but I wanted to know the status of reviews for 
this bug. @jyknight & @rnk, do you have further comments or need more time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Friendly ping. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D69868#1848327 , @MaskRay wrote:

> @rnk Thoughts on passing live-in formation (a bit similar to WinEH catchpad) 
> to `callbr` at the SelectionDAG stage?


To help with the review, here are some of the assumptions I'm making about the 
`INLINEASM_BR` and the copy block this patch creates:

1. The two blocks (CallBrBB and CopyBB) are "tightly coupled". I.e., you can't 
split the edge between them (not that you should want to).
2. The CopyBB block will *always* be the fall-through block. This implies that 
`INLINEASM_BR` is the only terminator in CallBrBB.
3. If one block moves, then both must move.

I would be happy to add code in the machine instruction verifier to ensure that 
these assumptions are met if you think it's necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

@rnk Thoughts on passing live-in formation (a bit similar to WinEH catchpad) to 
`callbr` at the SelectionDAG stage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

It seems that callbr (with output) will now be similar to a catchpad. It can 
set live-in physical register information. (See 
`test/CodeGen/X86/{seh-catch-all.ll,seh-exception-code.ll,wineh-coreclr.ll,wineh-exceptionpointer.ll}`)
 Is there any caveat doing this? Add @rnk to the attention list as the author 
of rL249492  and rL249786 
...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D69868#1836777 , @jyknight wrote:

> The idea of moving the copies to a new MachineBasicBlock seems a reasonable 
> solution. That said, it does mean there will be allocatable physical 
> registers which are live-in to the following block, which is generally not 
> allowed. As far as I can tell, I _think_ that should be fine in this 
> particular circumstance, but I'm a little uneasy that I might be missing some 
> reason why it'll be incorrect.


There are some passes (e.g. machine CSE; MachineCSE.cpp:692) that handle 
physical live-in registers before register allocation. Do those passes run 
after a pass which handles the physical live-in registers? (By "handle" I mean 
analyzes and/or modifies the machine IR so that physical live-ins are "okay".)

> I'm more uncomfortable with the scanning/splitting after-the-fact in 
> ScheduleDAGSDNodes.cpp, and then the successor lists subsequently being 
> incorrect. However, I wasn't immediately able to say what I'd suggest doing 
> instead, so I spent some time yesterday poking around with this patch to see 
> if I could find something that seemed better. I now believe it will be 
> cleaner to have the inline-asm tell SelectionDAGISel::FinishBasicBlock to 
> just emit the copies in another block, which we do in some other situations, 
> already. But, I'm still poking at that to see how it'll end up -- I can't say 
> I'm sure that's the right answer at the moment.

I looked at FinishBasicBlock at one point. I put it in EmitSchedule because I 
wanted to allow the basic block to go through any post processing that may 
occur. I can place it in FinishBasicBlock if you think it fits in there better.

As for the successor list, I justify it this way: The default behavior is 
exactly the same as executing the inline asm and continuing directly out as if 
it's straight line code. If INLINEASM_BR wasn't a terminator, we would add the 
COPYs directly after it and before the JMP. The only issue is whether the 
successors being on the copy block instead of the block containing the 
INLINEASM_BR would cause something to go wrong. Like you I was concerned about 
this. But I think that since the behavior is the same as if the two blocks were 
a single block (the assembly code isn't changed, etc.), and the fact that the 
successors being on the INLINEASM_BR block shouldn't affect any machine passes 
(i.e. no analysis or transformation should care, because the IR can't model 
potential branches by the asm), it should be okay. If you wish I could add a 
part to the machine instruction verifier to ensure that assumptions we're 
making are enforced.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

The idea of moving the copies to a new MachineBasicBlock seems a reasonable 
solution. That said, it does mean there will be allocatable physical registers 
which are live-in to the following block, which is generally not allowed. As 
far as I can tell, I _think_ that should be fine in this particular 
circumstance, but I'm a little uneasy that I might be missing some reason why 
it'll be incorrect.

I'm more uncomfortable with the scanning/splitting after-the-fact in 
ScheduleDAGSDNodes.cpp, and then the successor lists subsequently being 
incorrect. However, I wasn't immediately able to say what I'd suggest doing 
instead, so I spent some time yesterday poking around with this patch to see if 
I could find something that seemed better. I now believe it will be cleaner to 
have the inline-asm tell SelectionDAGISel::FinishBasicBlock to just emit the 
copies in another block, which we do in some other situations, already. But, 
I'm still poking at that to see how it'll end up -- I can't say I'm sure that's 
the right answer at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D69868#1832725 , @MaskRay wrote:

> In D69868#1832559 , @void wrote:
>
> > @jyknight Do you think you'll have time to review this patch this week? I'd 
> > like to get it into the 10.0 release if possible. :-)
>
>
> I volunteer as a reviewer:)


W00t! :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D69868#1832559 , @void wrote:

> @jyknight Do you think you'll have time to review this patch this week? I'd 
> like to get it into the 10.0 release if possible. :-)


I volunteer as a reviewer:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

@jyknight Do you think you'll have time to review this patch this week? I'd 
like to get it into the 10.0 release if possible. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 239231.
void added a comment.
Herald added a subscriber: MatzeB.

Split the machine basic block after an INLINEASM_BR instruction that has
outputs. The copies then end up in a separate block and the back end doesn't
have to deal with COPY instructions between two terminators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,160 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-LABEL: .Ltmp0: # Address of block that was removed by CodeGen
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  jmp .LBB1_4
+; CHECK-NEXT:  .LBB1_3: # %if.else
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  testl %esi, %edi
+; CHECK-NEXT:  jne .Ltmp2
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_4:
+; CHECK-NEXT:  movl %esi, %eax
+; CHECK-NEXT:  addl %edi, %eax
+; CHECK-NEXT:  .Ltmp2:
+; CHECK-NEXT:  # %bb.5: # %return
+; CHECK-LABEL: .Ltmp1: # Address of block that was removed by CodeGen
+define i32 @test2(i32 %out1, i32 %out2) {
+entry:
+  %cmp = icmp slt i32 %out1, %out2
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:  ; preds = %entry
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "={si},={di},r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.else:  ; preds = %entry
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "={si},={di},r,r,X,X,0,1,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i32 %out2, i8* blockaddress(@test2, %label_true), i8* blockaddress(@test2, %return), i32 %out1, i32 %out2)
+  to label %if.end [label %label_true, label %return]
+
+if.end:   ; preds = %if.else, %if.then
+  %.sink11 = phi { i32, i32 } [ %0, %if.then ], [ %1, %if.else ]
+  %asmresult3 = extractvalue { i32, i32 } %.sink11, 0
+  %asmresult4 = extractvalue { i32, i32 } %.sink11, 1
+  %add = add nsw i32 %asmresult4, %asmresult3
+  br label %return
+
+label_true:   ; preds = %if.else, %if.then
+  br label %return
+
+return:   ; preds = %if.then, %if.else, %label_true, %if.end
+  %retval.0 = phi i32 [ %add, %if.end ], [ -2, %label_true ], [ -1, %if.else ], [ -1, %if.then ]
+  ret i32 %retval.0
+}
+
+; CHECK-LABEL: test3:
+; CHECK:   # 

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 239026.
void added a comment.

Update so that each MBB has a list of indirect dests of INLINEASM_BR 
instructions.


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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
@@ -159,3 +159,54 @@
 cleanup:  ; preds = %asm.fallthrough, %quux
   ret void
 }
+
+define i32 @test5(i32 %out1, i32 %out2) {
+; Test 5 - asm-goto with output constraints.
+; CHECK-LABEL: test5:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:  movl $-1, %eax
+; CHECK-NEXT:  movl 4(%esp), %ecx
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %ecx
+; CHECK-NEXT:  testl %edx, %ecx
+; CHECK-NEXT:  jne .Ltmp6
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_1:
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  jne .Ltmp7
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_2:
+; CHECK-NEXT:  addl %edx, %ecx
+; CHECK-NEXT:  movl %ecx, %eax
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp6:
+; CHECK-NEXT:  .LBB4_3:
+; CHECK-NEXT:  movl $-2, %eax
+; CHECK-NEXT:  .Ltmp7:
+; CHECK-NEXT:  .LBB4_4:
+; CHECK-NEXT:  retl
+entry:
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "=r,=r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough [label %label_true, label %return]
+
+asm.fallthrough:  ; preds = %entry
+  %asmresult = extractvalue { i32, i32 } %0, 0
+  %asmresult1 = extractvalue { i32, i32 } %0, 1
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "=r,=r,r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %asmresult, i32 %asmresult1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough2 [label %label_true, label %return]
+
+asm.fallthrough2: ; preds = %asm.fallthrough
+  %asmresult3 = extractvalue { i32, i32 } %1, 0
+  %asmresult4 = extractvalue { i32, i32 } %1, 1
+  %add = add nsw i32 %asmresult3, %asmresult4
+  br label %return
+
+label_true:   ; preds = %asm.fallthrough, %entry
+  br label %return
+
+return:   ; preds = %entry, %asm.fallthrough, %label_true, %asm.fallthrough2
+  %retval.0 = phi i32 [ %add, %asm.fallthrough2 ], [ -2, %label_true ], [ -1, %asm.fallthrough ], [ -1, %entry ]
+  ret i32 %retval.0
+}
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,118 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp0:
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  jmp .LBB1_4
+; 

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void marked an inline comment as done.
void added inline comments.



Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701
+  } else if (MBB->succ_size() == LandingPadSuccs.size() ||
+ MBB->succ_size() == IndirectPadSuccs.size()) {
 // It's possible that the block legitimately ends with a noreturn

void wrote:
> jyknight wrote:
> > void wrote:
> > > jyknight wrote:
> > > > This isn't correct.
> > > > 
> > > > This line here, is looking at a block which doesn't end in a jump to a 
> > > > successor. So, it's trying to verify that the successor list makes 
> > > > sense in that context.
> > > > 
> > > > The unstated assumption in the code is that the only successors will be 
> > > > landing pads. Instead of actually checking each one, instead it just 
> > > > checks that the count is the number of landing pads, with the 
> > > > assumption that all the successors should be landing pads, and that all 
> > > > the landing pads should be successors.
> > > > 
> > > > The next clause is then checking for the case where there's a 
> > > > fallthrough to the next block. In that case, the successors should've 
> > > > been all the landing pads, and the single fallthrough block.
> > > > 
> > > > Adding similar code to check for the number of callbr targets doesn't 
> > > > really make sense. It's certainly not the case that all callbr targets 
> > > > are targets of all 
> > > > callbr instructions. And even if it was, this still wouldn't be 
> > > > counting things correctly.
> > > > 
> > > > 
> > > > However -- I think i'd expect analyzeBranch to error out (returning 
> > > > true) when confronted by a callbr instruction, because it cannot 
> > > > actually tell what's going on there. If that were the case, nothing in 
> > > > this block should even be invoked. But I guess that's probably not 
> > > > happening, due to the terminator being followed by non-terminators.
> > > > 
> > > > That seems likely to be a problem that needs to be fixed. (And if that 
> > > > is fixed, I think the changes here aren't needed anymore)
> > > > 
> > > > 
> > > Your comment is very confusing. Could you please give an example of where 
> > > this fails?
> > Sorry about that, I should've delimited the parts of that message better...
> > Basically:
> > - Paragraphs 2-4 are describing why the code before this patch appears to 
> > be correct for landing pad, even though it's taking some shortcuts and 
> > making some non-obvious assumptions.
> > - Paragraph 5 ("Adding similar code"...) is why it's not correct for callbr.
> > - Paragraph 6-7 are how I'd suggest to resolve it.
> > 
> > 
> > I believe the code as of your patch will fail validation if you have a 
> > callbr instruction which has a normal-successor block which is an indirect 
> > target of a *different* callbr in the function.
> > 
> > I believe it'll also fail if you have any landing-pad successors, since 
> > those aren't being added to the count of expected successors, but rather 
> > checked separately.
> > 
> > But more seriously than these potential verifier failures, I expect that 
> > analyzeBranch returning wrong answers (in that it may report that a block 
> > unconditionally-jumps to a successor, while it really has both a callbr and 
> > jump, separated by the non-terminator copies) will cause miscompilation. 
> > I'm not sure exactly how that will exhibit, but I'm pretty sure it's not 
> > going to be good.
> > 
> > And, if analyzeBranch properly said "no idea" when confronted by callbr 
> > control flow, then this code in the verifier wouldn't be reached.
> I didn't need a delineation of the parts of the comment. I needed a clearer 
> description of what your concern is, and to give an example of code that 
> fails here.
> 
> This bit of code is simply saying that if the block containing the 
> `INLINEASM_BR` doesn't end with a `BR` instruction, then the number of its 
> successors should be equal to the number of indirect successors. This is 
> correct, as it's not valid to have a duplicate label used in a `callbr` 
> instruction:
> 
> ```
> $ llc -o /dev/null x.ll
> Duplicate callbr destination!
>   %3 = callbr i32 asm sideeffect "testl $0, $0; testl $1, $1; jne ${2:l}", 
> "={si},r,X,0,~{dirflag},~{fpsr},~{flags}"(i32 %2, i8* blockaddress(@test1, 
> %asm.fallthrough), i32 %1) #2
>   to label %asm.fallthrough [label %asm.fallthrough], !srcloc !6
> ./bin/llc: x.ll: error: input module is broken!
> ```
> 
> A `callbr` with a normal successor block that is the indirect target of a 
> different `callbr` isn't really relevant here, unless I'm misunderstanding 
> what `analyzeBranch` returns. There would be two situations:
> 
> 1. The MBB ends in a fallthrough, which is the case I mentioned above, or
> 
> 2. The MBB ends in a `BR` instruction, in which case it won't be in this 
> block of code, but the block below.
> 
> If `analyzeBranch` is not taking into account potential `COPY` instructions 
> 

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

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



Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701
+  } else if (MBB->succ_size() == LandingPadSuccs.size() ||
+ MBB->succ_size() == IndirectPadSuccs.size()) {
 // It's possible that the block legitimately ends with a noreturn

jyknight wrote:
> void wrote:
> > jyknight wrote:
> > > This isn't correct.
> > > 
> > > This line here, is looking at a block which doesn't end in a jump to a 
> > > successor. So, it's trying to verify that the successor list makes sense 
> > > in that context.
> > > 
> > > The unstated assumption in the code is that the only successors will be 
> > > landing pads. Instead of actually checking each one, instead it just 
> > > checks that the count is the number of landing pads, with the assumption 
> > > that all the successors should be landing pads, and that all the landing 
> > > pads should be successors.
> > > 
> > > The next clause is then checking for the case where there's a fallthrough 
> > > to the next block. In that case, the successors should've been all the 
> > > landing pads, and the single fallthrough block.
> > > 
> > > Adding similar code to check for the number of callbr targets doesn't 
> > > really make sense. It's certainly not the case that all callbr targets 
> > > are targets of all 
> > > callbr instructions. And even if it was, this still wouldn't be counting 
> > > things correctly.
> > > 
> > > 
> > > However -- I think i'd expect analyzeBranch to error out (returning true) 
> > > when confronted by a callbr instruction, because it cannot actually tell 
> > > what's going on there. If that were the case, nothing in this block 
> > > should even be invoked. But I guess that's probably not happening, due to 
> > > the terminator being followed by non-terminators.
> > > 
> > > That seems likely to be a problem that needs to be fixed. (And if that is 
> > > fixed, I think the changes here aren't needed anymore)
> > > 
> > > 
> > Your comment is very confusing. Could you please give an example of where 
> > this fails?
> Sorry about that, I should've delimited the parts of that message better...
> Basically:
> - Paragraphs 2-4 are describing why the code before this patch appears to be 
> correct for landing pad, even though it's taking some shortcuts and making 
> some non-obvious assumptions.
> - Paragraph 5 ("Adding similar code"...) is why it's not correct for callbr.
> - Paragraph 6-7 are how I'd suggest to resolve it.
> 
> 
> I believe the code as of your patch will fail validation if you have a callbr 
> instruction which has a normal-successor block which is an indirect target of 
> a *different* callbr in the function.
> 
> I believe it'll also fail if you have any landing-pad successors, since those 
> aren't being added to the count of expected successors, but rather checked 
> separately.
> 
> But more seriously than these potential verifier failures, I expect that 
> analyzeBranch returning wrong answers (in that it may report that a block 
> unconditionally-jumps to a successor, while it really has both a callbr and 
> jump, separated by the non-terminator copies) will cause miscompilation. I'm 
> not sure exactly how that will exhibit, but I'm pretty sure it's not going to 
> be good.
> 
> And, if analyzeBranch properly said "no idea" when confronted by callbr 
> control flow, then this code in the verifier wouldn't be reached.
I didn't need a delineation of the parts of the comment. I needed a clearer 
description of what your concern is, and to give an example of code that fails 
here.

This bit of code is simply saying that if the block containing the 
`INLINEASM_BR` doesn't end with a `BR` instruction, then the number of its 
successors should be equal to the number of indirect successors. This is 
correct, as it's not valid to have a duplicate label used in a `callbr` 
instruction:

```
$ llc -o /dev/null x.ll
Duplicate callbr destination!
  %3 = callbr i32 asm sideeffect "testl $0, $0; testl $1, $1; jne ${2:l}", 
"={si},r,X,0,~{dirflag},~{fpsr},~{flags}"(i32 %2, i8* blockaddress(@test1, 
%asm.fallthrough), i32 %1) #2
  to label %asm.fallthrough [label %asm.fallthrough], !srcloc !6
./bin/llc: x.ll: error: input module is broken!
```

A `callbr` with a normal successor block that is the indirect target of a 
different `callbr` isn't really relevant here, unless I'm misunderstanding what 
`analyzeBranch` returns. There would be two situations:

1. The MBB ends in a fallthrough, which is the case I mentioned above, or

2. The MBB ends in a `BR` instruction, in which case it won't be in this block 
of code, but the block below.

If `analyzeBranch` is not taking into account potential `COPY` instructions 
between `INLINEASM_BR` and `BR`, then it needs to be addressed there (I'll 
verify that it is). I *do* know that this code is reached by the verifier, so 
it handles it to some degree. :-)

[PATCH] D69868: Allow "callbr" to return non-void values

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701
+  } else if (MBB->succ_size() == LandingPadSuccs.size() ||
+ MBB->succ_size() == IndirectPadSuccs.size()) {
 // It's possible that the block legitimately ends with a noreturn

void wrote:
> jyknight wrote:
> > This isn't correct.
> > 
> > This line here, is looking at a block which doesn't end in a jump to a 
> > successor. So, it's trying to verify that the successor list makes sense in 
> > that context.
> > 
> > The unstated assumption in the code is that the only successors will be 
> > landing pads. Instead of actually checking each one, instead it just checks 
> > that the count is the number of landing pads, with the assumption that all 
> > the successors should be landing pads, and that all the landing pads should 
> > be successors.
> > 
> > The next clause is then checking for the case where there's a fallthrough 
> > to the next block. In that case, the successors should've been all the 
> > landing pads, and the single fallthrough block.
> > 
> > Adding similar code to check for the number of callbr targets doesn't 
> > really make sense. It's certainly not the case that all callbr targets are 
> > targets of all 
> > callbr instructions. And even if it was, this still wouldn't be counting 
> > things correctly.
> > 
> > 
> > However -- I think i'd expect analyzeBranch to error out (returning true) 
> > when confronted by a callbr instruction, because it cannot actually tell 
> > what's going on there. If that were the case, nothing in this block should 
> > even be invoked. But I guess that's probably not happening, due to the 
> > terminator being followed by non-terminators.
> > 
> > That seems likely to be a problem that needs to be fixed. (And if that is 
> > fixed, I think the changes here aren't needed anymore)
> > 
> > 
> Your comment is very confusing. Could you please give an example of where 
> this fails?
Sorry about that, I should've delimited the parts of that message better...
Basically:
- Paragraphs 2-4 are describing why the code before this patch appears to be 
correct for landing pad, even though it's taking some shortcuts and making some 
non-obvious assumptions.
- Paragraph 5 ("Adding similar code"...) is why it's not correct for callbr.
- Paragraph 6-7 are how I'd suggest to resolve it.


I believe the code as of your patch will fail validation if you have a callbr 
instruction which has a normal-successor block which is an indirect target of a 
*different* callbr in the function.

I believe it'll also fail if you have any landing-pad successors, since those 
aren't being added to the count of expected successors, but rather checked 
separately.

But more seriously than these potential verifier failures, I expect that 
analyzeBranch returning wrong answers (in that it may report that a block 
unconditionally-jumps to a successor, while it really has both a callbr and 
jump, separated by the non-terminator copies) will cause miscompilation. I'm 
not sure exactly how that will exhibit, but I'm pretty sure it's not going to 
be good.

And, if analyzeBranch properly said "no idea" when confronted by callbr control 
flow, then this code in the verifier wouldn't be reached.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void marked an inline comment as done.
void added inline comments.



Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701
+  } else if (MBB->succ_size() == LandingPadSuccs.size() ||
+ MBB->succ_size() == IndirectPadSuccs.size()) {
 // It's possible that the block legitimately ends with a noreturn

jyknight wrote:
> This isn't correct.
> 
> This line here, is looking at a block which doesn't end in a jump to a 
> successor. So, it's trying to verify that the successor list makes sense in 
> that context.
> 
> The unstated assumption in the code is that the only successors will be 
> landing pads. Instead of actually checking each one, instead it just checks 
> that the count is the number of landing pads, with the assumption that all 
> the successors should be landing pads, and that all the landing pads should 
> be successors.
> 
> The next clause is then checking for the case where there's a fallthrough to 
> the next block. In that case, the successors should've been all the landing 
> pads, and the single fallthrough block.
> 
> Adding similar code to check for the number of callbr targets doesn't really 
> make sense. It's certainly not the case that all callbr targets are targets 
> of all 
> callbr instructions. And even if it was, this still wouldn't be counting 
> things correctly.
> 
> 
> However -- I think i'd expect analyzeBranch to error out (returning true) 
> when confronted by a callbr instruction, because it cannot actually tell 
> what's going on there. If that were the case, nothing in this block should 
> even be invoked. But I guess that's probably not happening, due to the 
> terminator being followed by non-terminators.
> 
> That seems likely to be a problem that needs to be fixed. (And if that is 
> fixed, I think the changes here aren't needed anymore)
> 
> 
Your comment is very confusing. Could you please give an example of where this 
fails?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701
+  } else if (MBB->succ_size() == LandingPadSuccs.size() ||
+ MBB->succ_size() == IndirectPadSuccs.size()) {
 // It's possible that the block legitimately ends with a noreturn

This isn't correct.

This line here, is looking at a block which doesn't end in a jump to a 
successor. So, it's trying to verify that the successor list makes sense in 
that context.

The unstated assumption in the code is that the only successors will be landing 
pads. Instead of actually checking each one, instead it just checks that the 
count is the number of landing pads, with the assumption that all the 
successors should be landing pads, and that all the landing pads should be 
successors.

The next clause is then checking for the case where there's a fallthrough to 
the next block. In that case, the successors should've been all the landing 
pads, and the single fallthrough block.

Adding similar code to check for the number of callbr targets doesn't really 
make sense. It's certainly not the case that all callbr targets are targets of 
all 
callbr instructions. And even if it was, this still wouldn't be counting things 
correctly.


However -- I think i'd expect analyzeBranch to error out (returning true) when 
confronted by a callbr instruction, because it cannot actually tell what's 
going on there. If that were the case, nothing in this block should even be 
invoked. But I guess that's probably not happening, due to the terminator being 
followed by non-terminators.

That seems likely to be a problem that needs to be fixed. (And if that is 
fixed, I think the changes here aren't needed anymore)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 237224.
void added a comment.

The plus constraints are now at the end of the input/output/label list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
@@ -159,3 +159,54 @@
 cleanup:  ; preds = %asm.fallthrough, %quux
   ret void
 }
+
+define i32 @test5(i32 %out1, i32 %out2) {
+; Test 5 - asm-goto with output constraints.
+; CHECK-LABEL: test5:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:  movl $-1, %eax
+; CHECK-NEXT:  movl 4(%esp), %ecx
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %ecx
+; CHECK-NEXT:  testl %edx, %ecx
+; CHECK-NEXT:  jne .Ltmp6
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_1:
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  jne .Ltmp7
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_2:
+; CHECK-NEXT:  addl %edx, %ecx
+; CHECK-NEXT:  movl %ecx, %eax
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp6:
+; CHECK-NEXT:  .LBB4_3:
+; CHECK-NEXT:  movl $-2, %eax
+; CHECK-NEXT:  .Ltmp7:
+; CHECK-NEXT:  .LBB4_4:
+; CHECK-NEXT:  retl
+entry:
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "=r,=r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough [label %label_true, label %return]
+
+asm.fallthrough:  ; preds = %entry
+  %asmresult = extractvalue { i32, i32 } %0, 0
+  %asmresult1 = extractvalue { i32, i32 } %0, 1
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "=r,=r,r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %asmresult, i32 %asmresult1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough2 [label %label_true, label %return]
+
+asm.fallthrough2: ; preds = %asm.fallthrough
+  %asmresult3 = extractvalue { i32, i32 } %1, 0
+  %asmresult4 = extractvalue { i32, i32 } %1, 1
+  %add = add nsw i32 %asmresult3, %asmresult4
+  br label %return
+
+label_true:   ; preds = %asm.fallthrough, %entry
+  br label %return
+
+return:   ; preds = %entry, %asm.fallthrough, %label_true, %asm.fallthrough2
+  %retval.0 = phi i32 [ %add, %asm.fallthrough2 ], [ -2, %label_true ], [ -1, %asm.fallthrough ], [ -1, %entry ]
+  ret i32 %retval.0
+}
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,118 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp0:
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 236716.
void added a comment.

Missed a change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
@@ -159,3 +159,54 @@
 cleanup:  ; preds = %asm.fallthrough, %quux
   ret void
 }
+
+define i32 @test5(i32 %out1, i32 %out2) {
+; Test 5 - asm-goto with output constraints.
+; CHECK-LABEL: test5:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:  movl $-1, %eax
+; CHECK-NEXT:  movl 4(%esp), %ecx
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %ecx
+; CHECK-NEXT:  testl %edx, %ecx
+; CHECK-NEXT:  jne .Ltmp6
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_1:
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  jne .Ltmp7
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_2:
+; CHECK-NEXT:  addl %edx, %ecx
+; CHECK-NEXT:  movl %ecx, %eax
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp6:
+; CHECK-NEXT:  .LBB4_3:
+; CHECK-NEXT:  movl $-2, %eax
+; CHECK-NEXT:  .Ltmp7:
+; CHECK-NEXT:  .LBB4_4:
+; CHECK-NEXT:  retl
+entry:
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "=r,=r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough [label %label_true, label %return]
+
+asm.fallthrough:  ; preds = %entry
+  %asmresult = extractvalue { i32, i32 } %0, 0
+  %asmresult1 = extractvalue { i32, i32 } %0, 1
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "=r,=r,r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %asmresult, i32 %asmresult1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough2 [label %label_true, label %return]
+
+asm.fallthrough2: ; preds = %asm.fallthrough
+  %asmresult3 = extractvalue { i32, i32 } %1, 0
+  %asmresult4 = extractvalue { i32, i32 } %1, 1
+  %add = add nsw i32 %asmresult3, %asmresult4
+  br label %return
+
+label_true:   ; preds = %asm.fallthrough, %entry
+  br label %return
+
+return:   ; preds = %entry, %asm.fallthrough, %label_true, %asm.fallthrough2
+  %retval.0 = phi i32 [ %add, %asm.fallthrough2 ], [ -2, %label_true ], [ -1, %asm.fallthrough ], [ -1, %entry ]
+  ret i32 %retval.0
+}
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,118 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp0:
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  jmp .LBB1_4
+; CHECK-NEXT:  .LBB1_3: # %if.else
+; 

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 236710.
void added a comment.

Remove check that the successor may be in the indirect list of a callbr 
instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
@@ -159,3 +159,54 @@
 cleanup:  ; preds = %asm.fallthrough, %quux
   ret void
 }
+
+define i32 @test5(i32 %out1, i32 %out2) {
+; Test 5 - asm-goto with output constraints.
+; CHECK-LABEL: test5:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:  movl $-1, %eax
+; CHECK-NEXT:  movl 4(%esp), %ecx
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %ecx
+; CHECK-NEXT:  testl %edx, %ecx
+; CHECK-NEXT:  jne .Ltmp6
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_1:
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  jne .Ltmp7
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB4_2:
+; CHECK-NEXT:  addl %edx, %ecx
+; CHECK-NEXT:  movl %ecx, %eax
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp6:
+; CHECK-NEXT:  .LBB4_3:
+; CHECK-NEXT:  movl $-2, %eax
+; CHECK-NEXT:  .Ltmp7:
+; CHECK-NEXT:  .LBB3_4:
+; CHECK-NEXT:  retl
+entry:
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "=r,=r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough [label %label_true, label %return]
+
+asm.fallthrough:  ; preds = %entry
+  %asmresult = extractvalue { i32, i32 } %0, 0
+  %asmresult1 = extractvalue { i32, i32 } %0, 1
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "=r,=r,r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %asmresult, i32 %asmresult1, i8* blockaddress(@test5, %label_true), i8* blockaddress(@test5, %return))
+  to label %asm.fallthrough2 [label %label_true, label %return]
+
+asm.fallthrough2: ; preds = %asm.fallthrough
+  %asmresult3 = extractvalue { i32, i32 } %1, 0
+  %asmresult4 = extractvalue { i32, i32 } %1, 1
+  %add = add nsw i32 %asmresult3, %asmresult4
+  br label %return
+
+label_true:   ; preds = %asm.fallthrough, %entry
+  br label %return
+
+return:   ; preds = %entry, %asm.fallthrough, %label_true, %asm.fallthrough2
+  %retval.0 = phi i32 [ %add, %asm.fallthrough2 ], [ -2, %label_true ], [ -1, %asm.fallthrough ], [ -1, %entry ]
+  ret i32 %retval.0
+}
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,118 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp0:
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void marked 5 inline comments as done.
void added inline comments.



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1115-1116
+  if (Succ->hasAddressTaken())
+if (auto *bb = Succ->getBasicBlock())
+  if (auto *cbr = dyn_cast(getBasicBlock()->getTerminator()))
+if (cbr->getDefaultDest() != bb)

nickdesaulniers wrote:
> I think it may be worthwhile to swap these conditions with each other, too.  
> It's highly unlikely the BB is terminated with a `CallBrInst`, yet highly 
> likely the MBB refers to a BB.
Okay. No one likes this addition. I'm going to remove it and just check 
`IsInlineAsmBrIndirectTarget`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: MaskRay, echristo.
rnk added a comment.

More reviewers: +@echristo @MaskRay




Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:133
+  /// Indicate that this basic block is the indirect dest of an INLINEASM_BR.
+  bool IsInlineAsmBrIndirectPad = false;
+

I think "IsInlineAsmBrIndirectTarget" would be better. I think of "Pad" as 
being something EH related, it's usually a "landing pad" or "eh pad".



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1114
+  // Don't do it in this generic function.
+  if (auto *bb = Succ->getBasicBlock())
+if (auto *cbr = dyn_cast(getBasicBlock()->getTerminator()))

rnk wrote:
> As a minor efficiency golf thing, I would start off this chain of checks with 
> `if (Succ->hasAddressTaken())`. In most cases, which will handle splitting 
> the critical edge to the fall through destination, which will not be marked 
> as address taken.
Somehow I missed that you added isInlineAsmBrIndirectPad, you could use that 
here instead of hasAddressTaken, and it would be more precise.



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1115
+  if (auto *bb = Succ->getBasicBlock())
+if (auto *cbr = dyn_cast(getBasicBlock()->getTerminator()))
+  if (cbr->getDefaultDest() != bb)

void wrote:
> rnk wrote:
> > This code really should be looking at the machine IR to know if there is a 
> > callbr. I think INLINEASM_BR is target independent, so you really can just 
> > loop through the MachineInstrs looking for it, and then look for the MBB in 
> > the operand list. I wonder if we should have a flag on the MBB to indicate 
> > that it ends in an INLINEASM_BR, since those break the MIR invariant that 
> > terminators are grouped together at the end of the block.
> The problem is that I need to between the indirect branches and the default 
> one from the `callbr` instruction. It's way more cumbersome to do that with 
> the `INLINEASM_BR` instruction. If you feel strongly about it I'll make the 
> change, but the `INLINEASM_BR` instruction is already strongly tied to 
> `callbr`. Do you expect it to change anytime in the future?
I don't think it will actually be that hard to implement. The MIR already knows 
a few things:
- After block layout, MBB->getFallthrough() will return the default 
destination. If non-null, you can use it.
- If getFallthrough() is null and this block terminated in callbr in IR, there 
must be an analyzable unconditional branch terminator (JMP or B).
- All other address taken successors must come from abnormal control flow, i.e. 
exception handling or callbr.

Actually, I'm surprised this utility doesn't return false after analyzeBranch 
below if Succ is not one of TBB or FBB.

In any case, if these ideas don't work out, I don't feel that strongly, but I 
feel obligated to ask you to try a variety of ways to make this work with just 
MIR info.



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1116
+if (auto *cbr = dyn_cast(getBasicBlock()->getTerminator()))
+  if (cbr->getDefaultDest() != bb)
+for (unsigned i = 0, e = cbr->getNumIndirectDests(); i != e; ++i)

xbolva00 wrote:
> void wrote:
> > nickdesaulniers wrote:
> > > void wrote:
> > > > rnk wrote:
> > > > > Is it possible for a BB to be both an indirect successor and a 
> > > > > fallthrough successor? I suppose that could be the case with the 
> > > > > Linux macro that gets the current PC.
> > > > > 
> > > > > In any case, it's probably safe to remove this condition, and then we 
> > > > > don't have to worry.
> > > > It is possible. I have a testcase for it in this patch. :-)
> > > But you don't have a test case for what @rnk is asking about. (Unsure if 
> > > it would be necessary).
> > > 
> > > I think @rnk is getting at this case from C:
> > > 
> > > ```
> > > void foo(void) {
> > >   asm goto ("#NICK":: "r"(&) :: hello);
> > > hello:
> > >   return;
> > > }
> > > ```
> > > Clang will emit this as:
> > > ```
> > > define dso_local void @foo() #0 {
> > > entry:
> > >   callbr void asm sideeffect "#NICK", 
> > > "r,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %hello), i8* 
> > > blockaddress(@foo, %hello)) #1
> > >   to label %asm.fallthrough [label %hello], !srcloc !2
> > > 
> > > asm.fallthrough:  ; preds = %entry
> > >   br label %hello
> > > 
> > > hello:; preds = 
> > > %asm.fallthrough, %entry
> > >   ret void
> > > }
> > > ```
> > > ie. the `blockaddress` is passed twice, once as the address of a label 
> > > (GNU C extension), once as the indirect destination of the `asm goto`.
> > > 
> > > So to @rnk 's question:
> > > > Is it possible for a BB to be both an indirect successor and a 
> > > > fallthrough successor?
> > > It is valid LLVM IR to have a BB be both; Clang today (or with 
> > > https://reviews.llvm.org/D69876) 

[PATCH] D69868: Allow "callbr" to return non-void values

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added subscribers: aaron.ballman, xbolva00.
xbolva00 added inline comments.



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1116
+if (auto *cbr = dyn_cast(getBasicBlock()->getTerminator()))
+  if (cbr->getDefaultDest() != bb)
+for (unsigned i = 0, e = cbr->getNumIndirectDests(); i != e; ++i)

void wrote:
> nickdesaulniers wrote:
> > void wrote:
> > > rnk wrote:
> > > > Is it possible for a BB to be both an indirect successor and a 
> > > > fallthrough successor? I suppose that could be the case with the Linux 
> > > > macro that gets the current PC.
> > > > 
> > > > In any case, it's probably safe to remove this condition, and then we 
> > > > don't have to worry.
> > > It is possible. I have a testcase for it in this patch. :-)
> > But you don't have a test case for what @rnk is asking about. (Unsure if it 
> > would be necessary).
> > 
> > I think @rnk is getting at this case from C:
> > 
> > ```
> > void foo(void) {
> >   asm goto ("#NICK":: "r"(&) :: hello);
> > hello:
> >   return;
> > }
> > ```
> > Clang will emit this as:
> > ```
> > define dso_local void @foo() #0 {
> > entry:
> >   callbr void asm sideeffect "#NICK", "r,X,~{dirflag},~{fpsr},~{flags}"(i8* 
> > blockaddress(@foo, %hello), i8* blockaddress(@foo, %hello)) #1
> >   to label %asm.fallthrough [label %hello], !srcloc !2
> > 
> > asm.fallthrough:  ; preds = %entry
> >   br label %hello
> > 
> > hello:; preds = 
> > %asm.fallthrough, %entry
> >   ret void
> > }
> > ```
> > ie. the `blockaddress` is passed twice, once as the address of a label (GNU 
> > C extension), once as the indirect destination of the `asm goto`.
> > 
> > So to @rnk 's question:
> > > Is it possible for a BB to be both an indirect successor and a 
> > > fallthrough successor?
> > It is valid LLVM IR to have a BB be both; Clang today (or with 
> > https://reviews.llvm.org/D69876) will not emit such formation (but could).
> > 
> > ie. imagine the above:
> > 
> > >  callbr void asm sideeffect "#NICK", 
> > > "r,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %hello), i8* 
> > > blockaddress(@foo, %hello)) #1
> > >   to label %asm.fallthrough [label %hello], !srcloc !2
> > 
> > to instead be:
> > 
> > >  callbr void asm sideeffect "#NICK", 
> > > "r,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %hello), i8* 
> > > blockaddress(@foo, %asm.fallthrough)) #1
> > >   to label %asm.fallthrough [label %asm.fallthrough], !srcloc !2
> > 
> > I still don't understand @rnk 's point about
> > > In any case, it's probably safe to remove this condition, and then we 
> > > don't have to worry.
> > though.
> I meant that I had a testcase that required the conditional he suggested I 
> remove. Sorry for the confusion.
> 
> You're right that we can generate the clang code where the fallthrough is the 
> same as the indirect. I didn't mean to imply that it wasn't the case. I can 
> add a testcase. I asked a question on the "cfe-dev" mailing list to determine 
> how I could identify the "fallthrough" block from the CFG. So far no one has 
> responded. Until I can determine that, I won't be able to handle that 
> properly with this conditional.
Maybe @aaron.ballman can answer your question related to “fallthrough”  block..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 234403.
void added a comment.

Fix bad update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/MachineBasicBlock.h
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp
  llvm/lib/CodeGen/MachineVerifier.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/test/CodeGen/X86/callbr-asm-outputs.ll
  llvm/test/CodeGen/X86/callbr-asm.ll

Index: llvm/test/CodeGen/X86/callbr-asm.ll
===
--- llvm/test/CodeGen/X86/callbr-asm.ll
+++ llvm/test/CodeGen/X86/callbr-asm.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
+; RUN: llc < %s -mtriple=i686-- -O3 -verify-machineinstrs | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
@@ -131,3 +131,54 @@
   %1 = load i32, i32* %a.addr, align 4
   ret i32 %1
 }
+
+define i32 @test4(i32 %out1, i32 %out2) {
+; Test 4 - asm-goto with output constraints.
+; CHECK-LABEL: test4:
+; CHECK:   # %bb.0: # %entry
+; CHECK-NEXT:  movl $-1, %eax
+; CHECK-NEXT:  movl 4(%esp), %ecx
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %ecx
+; CHECK-NEXT:  testl %edx, %ecx
+; CHECK-NEXT:  jne .Ltmp5
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB3_1:
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  testl %ecx, %edx
+; CHECK-NEXT:  jne .Ltmp6
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB3_2:
+; CHECK-NEXT:  addl %edx, %ecx
+; CHECK-NEXT:  movl %ecx, %eax
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp5:
+; CHECK-NEXT:  .LBB3_3:
+; CHECK-NEXT:  movl $-2, %eax
+; CHECK-NEXT:  .Ltmp6:
+; CHECK-NEXT:  .LBB3_4:
+; CHECK-NEXT:  retl
+entry:
+  %0 = callbr { i32, i32 } asm sideeffect "testl $0, $0; testl $1, $2; jne ${3:l}", "=r,=r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %out1, i8* blockaddress(@test4, %label_true), i8* blockaddress(@test4, %return))
+  to label %asm.fallthrough [label %label_true, label %return]
+
+asm.fallthrough:  ; preds = %entry
+  %asmresult = extractvalue { i32, i32 } %0, 0
+  %asmresult1 = extractvalue { i32, i32 } %0, 1
+  %1 = callbr { i32, i32 } asm sideeffect "testl $0, $1; testl $2, $3; jne ${5:l}", "=r,=r,r,r,X,X,~{dirflag},~{fpsr},~{flags}"(i32 %asmresult, i32 %asmresult1, i8* blockaddress(@test4, %label_true), i8* blockaddress(@test4, %return))
+  to label %asm.fallthrough2 [label %label_true, label %return]
+
+asm.fallthrough2: ; preds = %asm.fallthrough
+  %asmresult3 = extractvalue { i32, i32 } %1, 0
+  %asmresult4 = extractvalue { i32, i32 } %1, 1
+  %add = add nsw i32 %asmresult3, %asmresult4
+  br label %return
+
+label_true:   ; preds = %asm.fallthrough, %entry
+  br label %return
+
+return:   ; preds = %entry, %asm.fallthrough, %label_true, %asm.fallthrough2
+  %retval.0 = phi i32 [ %add, %asm.fallthrough2 ], [ -2, %label_true ], [ -1, %asm.fallthrough ], [ -1, %entry ]
+  ret i32 %retval.0
+}
Index: llvm/test/CodeGen/X86/callbr-asm-outputs.ll
===
--- llvm/test/CodeGen/X86/callbr-asm-outputs.ll
+++ llvm/test/CodeGen/X86/callbr-asm-outputs.ll
@@ -1,18 +1,118 @@
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
 
-; CHECK: error: asm-goto outputs not supported
+; A test for asm-goto output
 
-; A test for asm-goto output prohibition
-
-define i32 @test(i32 %a) {
+; CHECK-LABEL: test1:
+; CHECK:   movl 4(%esp), %eax
+; CHECK-NEXT:  addl $4, %eax
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  xorl %eax, %eax
+; CHECK-NEXT:  jmp .Ltmp0
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB0_1:
+; CHECK-NEXT:  retl
+; CHECK-NEXT:  .Ltmp0:
+define i32 @test1(i32 %x) {
 entry:
-  %0 = add i32 %a, 4
-  %1 = callbr i32 asm "xorl $1, $1; jmp ${1:l}", "=,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %0, i8* blockaddress(@test, %fail)) to label %normal [label %fail]
+  %add = add nsw i32 %x, 4
+  %ret = callbr i32 asm "xorl $1, $0; jmp ${2:l}", "=r,r,X,~{dirflag},~{fpsr},~{flags}"(i32 %add, i8* blockaddress(@test1, %abnormal))
+  to label %normal [label %abnormal]
 
 normal:
-  ret i32 %1
+  ret i32 %ret
 
-fail:
+abnormal:
   ret i32 1
 }
+
+; CHECK-LABEL: test2:
+; CHECK:   # %bb.1: # %if.then
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  testl %esi, %esi
+; CHECK-NEXT:  testl %edi, %esi
+; CHECK-NEXT:  jne .Ltmp1
+; CHECK-NEXT:  #NO_APP
+; CHECK-NEXT:  .LBB1_2:
+; CHECK-NEXT:  jmp .LBB1_4
+; CHECK-NEXT:  .LBB1_3: # %if.else
+; CHECK-NEXT:  #APP
+; CHECK-NEXT:  

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:1116
+if (auto *cbr = dyn_cast(getBasicBlock()->getTerminator()))
+  if (cbr->getDefaultDest() != bb)
+for (unsigned i = 0, e = cbr->getNumIndirectDests(); i != e; ++i)

nickdesaulniers wrote:
> void wrote:
> > rnk wrote:
> > > Is it possible for a BB to be both an indirect successor and a 
> > > fallthrough successor? I suppose that could be the case with the Linux 
> > > macro that gets the current PC.
> > > 
> > > In any case, it's probably safe to remove this condition, and then we 
> > > don't have to worry.
> > It is possible. I have a testcase for it in this patch. :-)
> But you don't have a test case for what @rnk is asking about. (Unsure if it 
> would be necessary).
> 
> I think @rnk is getting at this case from C:
> 
> ```
> void foo(void) {
>   asm goto ("#NICK":: "r"(&) :: hello);
> hello:
>   return;
> }
> ```
> Clang will emit this as:
> ```
> define dso_local void @foo() #0 {
> entry:
>   callbr void asm sideeffect "#NICK", "r,X,~{dirflag},~{fpsr},~{flags}"(i8* 
> blockaddress(@foo, %hello), i8* blockaddress(@foo, %hello)) #1
>   to label %asm.fallthrough [label %hello], !srcloc !2
> 
> asm.fallthrough:  ; preds = %entry
>   br label %hello
> 
> hello:; preds = %asm.fallthrough, 
> %entry
>   ret void
> }
> ```
> ie. the `blockaddress` is passed twice, once as the address of a label (GNU C 
> extension), once as the indirect destination of the `asm goto`.
> 
> So to @rnk 's question:
> > Is it possible for a BB to be both an indirect successor and a fallthrough 
> > successor?
> It is valid LLVM IR to have a BB be both; Clang today (or with 
> https://reviews.llvm.org/D69876) will not emit such formation (but could).
> 
> ie. imagine the above:
> 
> >  callbr void asm sideeffect "#NICK", "r,X,~{dirflag},~{fpsr},~{flags}"(i8* 
> > blockaddress(@foo, %hello), i8* blockaddress(@foo, %hello)) #1
> >   to label %asm.fallthrough [label %hello], !srcloc !2
> 
> to instead be:
> 
> >  callbr void asm sideeffect "#NICK", "r,X,~{dirflag},~{fpsr},~{flags}"(i8* 
> > blockaddress(@foo, %hello), i8* blockaddress(@foo, %asm.fallthrough)) #1
> >   to label %asm.fallthrough [label %asm.fallthrough], !srcloc !2
> 
> I still don't understand @rnk 's point about
> > In any case, it's probably safe to remove this condition, and then we don't 
> > have to worry.
> though.
I meant that I had a testcase that required the conditional he suggested I 
remove. Sorry for the confusion.

You're right that we can generate the clang code where the fallthrough is the 
same as the indirect. I didn't mean to imply that it wasn't the case. I can add 
a testcase. I asked a question on the "cfe-dev" mailing list to determine how I 
could identify the "fallthrough" block from the CFG. So far no one has 
responded. Until I can determine that, I won't be able to handle that properly 
with this conditional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69868: Allow "callbr" to return non-void values

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 234402.
void marked 3 inline comments as done.
void added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/AST/Stmt.h
  clang/lib/AST/Stmt.cpp
  clang/lib/Analysis/UninitializedValues.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/Analysis/uninit-asm-goto.cpp
  clang/test/CodeGen/asm-goto.c
  clang/test/Parser/asm-goto.c
  clang/test/Parser/asm-goto.cpp
  clang/test/Sema/asm-goto.cpp
  llvm/lib/CodeGen/MachineBasicBlock.cpp

Index: llvm/lib/CodeGen/MachineBasicBlock.cpp
===
--- llvm/lib/CodeGen/MachineBasicBlock.cpp
+++ llvm/lib/CodeGen/MachineBasicBlock.cpp
@@ -1109,6 +1109,17 @@
   if (Succ->isEHPad())
 return false;
 
+  // Splitting the critical edge to a callbr's indirect block isn't advised.
+  // Don't do it in this generic function.
+  if (Succ->hasAddressTaken())
+if (auto *cbr = dyn_cast(getBasicBlock()->getTerminator()))
+  if (auto *bb = Succ->getBasicBlock())
+if (cbr->getDefaultDest() != bb)
+  if (llvm::any_of(cbr->getIndirectDests(), [&](const BasicBlock *succ){
+  return succ == bb;
+  }))
+return false;
+
   const MachineFunction *MF = getParent();
 
   // Performance might be harmed on HW that implements branching using exec mask
Index: clang/test/Sema/asm-goto.cpp
===
--- clang/test/Sema/asm-goto.cpp
+++ clang/test/Sema/asm-goto.cpp
@@ -1,53 +1,51 @@
 // RUN: %clang_cc1 %s -triple i386-pc-linux-gnu -verify -fsyntax-only
 // RUN: %clang_cc1 %s -triple x86_64-pc-linux-gnu -verify -fsyntax-only
 
-struct NonTrivial {
-  ~NonTrivial();
+struct S {
+  ~S();
   int f(int);
 private:
   int k;
 };
-void JumpDiagnostics(int n) {
-// expected-error@+1 {{cannot jump from this goto statement to its label}}
+void test1(int n) {
+  // expected-error@+1 {{cannot jump from this goto statement to its label}}
   goto DirectJump;
-// expected-note@+1 {{jump bypasses variable with a non-trivial destructor}}
-  NonTrivial tnp1;
+  // expected-note@+1 {{jump bypasses variable with a non-trivial destructor}}
+  S s1;
 
 DirectJump:
-// expected-error@+1 {{cannot jump from this asm goto statement to one of its possible targets}}
+  // expected-error@+1 {{cannot jump from this asm goto statement to one of its possible targets}}
   asm goto("jmp %l0;" Later);
-// expected-note@+1 {{jump bypasses variable with a non-trivial destructor}}
-  NonTrivial tnp2;
-// expected-note@+1 {{possible target of asm goto statement}}
+  // expected-note@+1 {{jump bypasses variable with a non-trivial destructor}}
+  S s2;
+  // expected-note@+1 {{possible target of asm goto statement}}
 Later:
   return;
 }
 
-struct S { ~S(); };
-void foo(int a) {
+struct T { ~T(); };
+void test2(int a) {
   if (a) {
 FOO:
-// expected-note@+2 {{jump exits scope of variable with non-trivial destructor}}
-// expected-note@+1 {{jump exits scope of variable with non-trivial destructor}}
-S s;
-void *p = &
-// expected-error@+1 {{cannot jump from this asm goto statement to one of its possible targets}}
-  asm goto("jmp %l0;" BAR);
-// expected-error@+1 {{cannot jump from this indirect goto statement to one of its possible targets}}
+// expected-note@+2 {{jump exits scope of variable with non-trivial destructor}}
+// expected-note@+1 {{jump exits scope of variable with non-trivial destructor}}
+T t;
+void *p = &
+  // expected-error@+1 {{cannot jump from this asm goto statement to one of its possible targets}}
+  asm goto("jmp %l0;" bar);
+// expected-error@+1 {{cannot jump from this indirect goto statement to one of its possible targets}}
 goto *p;
 p = &
 goto *p;
 return;
   }
-// expected-note@+2 {{possible target of asm goto statement}}
-// expected-note@+1 {{possible target of indirect goto statement}}
-BAR:
+  // expected-note@+2 {{possible target of asm goto statement}}
+  // expected-note@+1 {{possible target of indirect goto statement}}
+bar:
   return;
 }
 
-
-//Asm goto:
-int test16(int n)
+int test3(int n)
 {
   // expected-error@+2 {{cannot jump from this asm goto statement to one of its possible targets}}
   // expected-error@+1 {{cannot jump from this asm goto statement to one of its possible targets}}
@@ -57,7 +55,7 @@
   return ({int a[n];label_true: 2;});
   // expected-note@+1 {{jump bypasses initialization of variable length array}}
   int b[n];
-// expected-note@+1 {{possible target of asm goto statement}}
+  // expected-note@+1 {{possible target of asm goto statement}}
 loop:
   return 0;
 }
Index: clang/test/Parser/asm-goto.cpp