Page MenuHomePhabricator

Allow "callbr" to return non-void values
Needs ReviewPublic

Authored by void on Nov 5 2019, 2:34 PM.

Details

Summary

Remove the restrictions preventing "callbr" from returning non-void values.
The value returned by "callbr" that are used on "indirect" paths are poisoned.

Event Timeline

void created this revision.Nov 5 2019, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2019, 2:34 PM
void updated this revision to Diff 228306.Nov 7 2019, 1:45 PM

Update language reference.

nickdesaulniers added inline comments.Nov 7 2019, 2:42 PM
llvm/docs/LangRef.rst
7149

Rather than the distinction between "normal" and "other," can we use the terms "fallthrough" and "indirect?"

7152

I assume that's the WIP part? Otherwise there's nothing else here that explicitly adds poison from what I can tell.

void marked an inline comment as done.Nov 7 2019, 4:31 PM
void added inline comments.
llvm/docs/LangRef.rst
7152

You're able to have the "fallthrough" listed in the "indirect" list as well. I'm just pointing out that if the value hasn't been calculated and shoved into the output object (register, etc.) then using that value in the fallthrough branch causes a poisoned value.

void updated this revision to Diff 228336.Nov 7 2019, 4:31 PM

Update description to use "fallthrough" and "indirect"

void updated this revision to Diff 228596.Sun, Nov 10, 1:15 AM

Add testcase for multiple output constraints.

void edited the summary of this revision. (Show Details)Sun, Nov 10, 6:25 PM

This change is now ready for review. PTAL.

void updated this revision to Diff 228813.Mon, Nov 11, 10:32 PM

Don't split critical edges into a callbr indirect destination.

void updated this revision to Diff 228820.Mon, Nov 11, 11:56 PM

Temporary change to determine if a block is used in a callbr.

void updated this revision to Diff 228926.Tue, Nov 12, 11:30 AM

Simplify check that a basic block is the target of an indirect jump from callbr
and add testcase for this.

void added a comment.Thu, Nov 14, 10:29 PM

Friendly ping. :-)

nickdesaulniers requested changes to this revision.Mon, Nov 18, 10:51 AM

The value returned by "callbr" is only valid on the "normal" path. Return

values used on "abnormal" paths are poisoned.

Please edit the revision comment to use the terms "fallthrough" and "indirect" targets or paths, rather than "normal" and "abnormal."

llvm/docs/LangRef.rst
7150

I still don't understand how the poison value gets added. Can you please clarify, or maybe add a test case that does?

7150

If the outputs are only valid on the fallthrough, will that allow us to build the extension to the existing asm goto that has been asked for? I worry that if we implement this restriction, it may not be fully useful unless the outputs on all paths are valid. Do you have a link to the previous discussion about this?

llvm/lib/IR/BasicBlock.cpp
481 ↗(On Diff #228926)

In the past, I've been shot down for adding to BasicBlock. Can this be implemented as a method on a CallBrInst that accepts a BasicBlock?

487 ↗(On Diff #228926)

PI is only iterating BasicBlocks in this Function, right? Can't callbr refer to labels in other functions (unlike asm goto in C, due to scoping)?

This revision now requires changes to proceed.Mon, Nov 18, 10:51 AM
void edited the summary of this revision. (Show Details)Mon, Nov 18, 1:00 PM
void updated this revision to Diff 229918.Mon, Nov 18, 1:51 PM
void marked 4 inline comments as done.

Don't add to BasicBlock, but just loop directly.

llvm/docs/LangRef.rst
7150

I still don't understand how the poison value gets added. Can you please
clarify, or maybe add a test case that does?

Poison values are a result of execution, not specified directly. So during execution, the value becomes poison if used in the indirect block, but not if used in the fallthrough. It's just fancy language for saying "don't do that!" :-)

From the docs:

Poison value behavior is defined in terms of value dependence:
  ...
  * An instruction control-depends on a terminator instruction if the terminator instruction has multiple successors and the instruction is always executed when control transfers to one of the successors, and may not be executed when control is transferred to another.
7150

After going back and forth with James and Hal, I now believe that having the value valid only on the fallthrough path will be sufficient for most purposes (if not all). However, this implementation doesn't restrict us from relaxing it in the future, but I would like to see a use case for that before we sink a lot of time into it.

Below is a link to the start of the thread.

http://lists.llvm.org/pipermail/llvm-dev/2019-June/133428.html

llvm/lib/IR/BasicBlock.cpp
481 ↗(On Diff #228926)

Better, I'll just inline it. :-)

487 ↗(On Diff #228926)

I don't believe it can refer to labels in other functions. Anyway, I changed this so that this function is gone.

void added a comment.Sun, Nov 24, 1:59 PM

Ping? :-)

Ping? :-)

Sorry, busy week going into Thanksgiving holiday. I really think this needs more time to bake in code review and input from @jyknight (and maybe @rnk ). Particularly, re-reading through the thread (https://lists.llvm.org/pipermail/llvm-dev/2019-June/133428.html), I'm curious to see more tests for cases that we don't expect to work, particularly for the clang patch I'm curious if we can somehow warn the user?

Should you add a test case here for the case @jyknight describes in: https://lists.llvm.org/pipermail/llvm-dev/2019-June/133431.html (ie. differing output constraints)?

void updated this revision to Diff 231008.Mon, Nov 25, 11:17 PM

Add testcase for callbrs with different output constraints jumping to the same
default location.

rnk added inline comments.Tue, Dec 3, 3:48 PM
llvm/lib/CodeGen/MachineBasicBlock.cpp
1114

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.

1115

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.

1116

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.

llvm/test/CodeGen/X86/callbr-asm-outputs.ll
1

Please add -verify-machineinstrs to the tests, since that isn't usually on by default, and it may highlight some latent verifier issues I don't know about.

llvm/test/CodeGen/X86/callbr-asm.ll
2

Please add -verify-machineinstrs.

void updated this revision to Diff 232049.Wed, Dec 4, 12:45 AM
void marked 6 inline comments as done.

Add "verify-machineinstrs" flag to callbr tests. Relax some of the verification
tests to account for callbrs.

llvm/lib/CodeGen/MachineBasicBlock.cpp
1115

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?

1116

It is possible. I have a testcase for it in this patch. :-)