[PATCH] D53765: [RFC prototype] Implementation of asm-goto support in LLVM
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 29 01:51:57 PST 2019
chandlerc added a comment.
Sorry it took so long, but made a full pass through this. Just want to point out that this patch is already in really great shape due to the huge amount of work from the original author, Craig taking it over, and the multiple rounds of review from Bill and Nick among others.
Most of my comments are pretty superficial and not hard to address I suspect. I have one big question and one big comment. I've referenced these in-line below as I went through the code but wanted to call them out at a highlevel:
1. Why do we need the block address machine operand structure? I'm not saying to get rid of it (yet), I just don't understand the need to support both it and direct MBB operands.
2. Why do we need to special case asm-goto targets in MI? I feel like this should somewhat fall out of the handling of address-taken basic blocks that can be jumped to somewhat opaquely. It would be really nice if there were a relatively common way of handling these rather than special casing the targets of asm-goto everywhere when it feels like the fundamental special treatment isn't actually about asm-goto at all. But maybe I've missed the critical thing that actually makes it special to asm-goto. If so, just help mme spot it1 =D
I haven't looked as closely at the actual optimizer changes yet as I'd like, but I think those are by-and-large going to be pretty straightforward.
================
Comment at: include/llvm/IR/CallSite.h:53
+ typename ValTy = const Value, typename UserTy = const User,
+ typename UseTy = const Use, typename InstrTy = const Instruction,
typename CallTy = const CallInst,
----------------
Bleh. We should just finish killing CallSite. But I guess this patch shouldn't wait on that.
================
Comment at: include/llvm/IR/CallSite.h:89-91
+ /// Return true if a InvokeInst is enclosed. !I.getInt() may also signify a
+ /// NULL instruction pointer, so check that.
bool isInvoke() const { return getInstruction() && !I.getInt(); }
----------------
FWIW, `I.getInt() == 0` seems way more clear.
================
Comment at: include/llvm/IR/CallSite.h:259-264
+ assert(getInstruction() && "Not a call, invoke or callbr instruction!");
+ if (isCallBr())
+ return (*this)->op_end() - 2 -
+ cast<CallBrInst>(getInstruction())->getNumTransfers();
+ else
+ return (*this)->op_end() - (isCall() ? 1 : 3);
----------------
Can this be simplified by just delegating to `CallBase`? The amount of magic constants / implicit contracts here worries me.
================
Comment at: include/llvm/IR/CallSite.h:321-324
if (isCall())
return cast<CallInst>(getInstruction())->isInlineAsm();
+ if (isCallBr())
+ return cast<CallBrInst>(getInstruction())->isInlineAsm();
----------------
Can we just sink this into `CallBase`?
================
Comment at: include/llvm/IR/CallSite.h:600-608
const Instruction *II = getInstruction();
// Since this is actually a getter that "looks like" a setter, don't use the
// above macros to avoid confusion.
if (isCall())
cast<CallInst>(II)->getOperandBundlesAsDefs(Defs);
+ else if (isCallBr())
+ cast<CallBrInst>(II)->getOperandBundlesAsDefs(Defs);
----------------
Delegate to `CallBase`?
================
Comment at: include/llvm/IR/InstVisitor.h:273-279
// The next level delegation for `CallBase` is slightly more complex in order
// to support visiting cases where the call is also a terminator.
RetTy visitCallBase(CallBase &I) {
if (isa<InvokeInst>(I))
return static_cast<SubClass *>(this)->visitTerminator(I);
DELEGATE(Instruction);
----------------
Need to update this code path.
================
Comment at: include/llvm/IR/Instructions.h:4036-4038
+ /// Check if the CallBr is an asm-goto
+ bool isInlineAsm() const { return isa<InlineAsm>(getCalledValue()); }
+
----------------
See above, this I think should sink to `CallBase`.
================
Comment at: lib/AsmParser/LLParser.cpp:6197-6198
+/// ParseCallBr
+/// ::= 'callbr' OptionalCallingConv OptionalAttrs Type Value ParamList
+/// OptionalAttrs 'to' TypeAndValue 'or jump' '[' LabelList ']'
+bool LLParser::ParseCallBr(Instruction *&Inst, PerFunctionState &PFS) {
----------------
bundles missing here?
================
Comment at: lib/AsmParser/LLParser.cpp:6197-6198
+/// ParseCallBr
+/// ::= 'callbr' OptionalCallingConv OptionalAttrs Type Value ParamList
+/// OptionalAttrs 'to' TypeAndValue 'or jump' '[' LabelList ']'
+bool LLParser::ParseCallBr(Instruction *&Inst, PerFunctionState &PFS) {
----------------
chandlerc wrote:
> bundles missing here?
The syntax here diverges from both `switch` and `indirectbr` in surprising ways here. They don't use any keywords like `or jump` to introduce thi list of labels, they just directly put it there.
While abstractly, I somewhat like this syntax, I wonder if it would be better te *exactly* match the syntax for `switch` which has the same default label with a list of other labels structure to it. Consistency seems good here unless it causes problems?
================
Comment at: lib/AsmParser/LLParser.cpp:6250
+ // Pull out the types of all of the arguments...
+ std::vector<Type *> ParamTypes;
+ for (unsigned i = 0, e = ArgList.size(); i != e; ++i)
----------------
Not a small vector?
================
Comment at: lib/AsmParser/LLParser.cpp:6269
+ if (isa<InlineAsm>(Callee) && !Ty->getReturnType()->isVoidTy())
+ return Error(RetTypeLoc, "asm-goto outputs not supported");
+
----------------
Weird indentatino.
================
Comment at: lib/AsmParser/LLParser.cpp:6279
+ FunctionType::param_iterator E = Ty->param_end();
+ for (unsigned i = 0, e = ArgList.size(); i != e; ++i) {
+ Type *ExpectedTy = nullptr;
----------------
I'd use `int` and `llvm::seq`, but maybe better to remain consistent.
================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2973-2974
// If this is a landing pad, it isn't a fall through. If it has no preds,
- // then nothing falls through to it.
- if (MBB->isEHPad() || MBB->pred_empty())
+ // then nothing falls through to it. If it is an asm-goto target it isn't
+ // a fallthrough.
+ if (MBB->isEHPad() || MBB->pred_empty() || MBB->isAsmGotoTarget())
----------------
Isn't one of the targets a fallthrough? Maybe i've not made it far enough, and the default target is different from the target here.
================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp:439-443
+ if (MI->getOperand(OpNo).isBlockAddress()) {
+ const BlockAddress *BA = MI->getOperand(OpNo).getBlockAddress();
+ MCSymbol *Sym =AP->GetBlockAddressSymbol(BA);
+ Sym->print(OS, AP->MAI);
+ } else {
----------------
I'm a bit surprised we don't model this as an MBB operand to begin with if it *has* to be a direct block address anyways? Curious why.
================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1300
+
+ // FIXME: track probabilities.
+ MachineBasicBlock &ReturnMBB = getMBB(*ReturnBB);
----------------
This complex to thread through?
================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1246
+
+ // FIXME: support whatever these are.
+ if (I.countOperandBundlesOfType(LLVMContext::OB_deopt))
----------------
craig.topper wrote:
> void wrote:
> > "whether these are" what? :-)
> No I think the person who wrote this was throwing their hands up and saying they don't what that is.
Maybe have the verifier reject if we can't lower here? Regardless, the comment still seems to need some amount of fixing.
`deopt` bundles are a reasonable thing, and just not really a reasonable thing (at this stage) to support on callbr.
================
Comment at: lib/CodeGen/IndirectBrExpandPass.cpp:151-153
+ // FIXME: this part doesn't properly recognize other uses of blockaddress
+ // expressions, for instance, where they are used to pass labels to
+ // asm-goto. This part of the pass needs a rework.
----------------
I think we have to fix this before this goes very far. I don't actually know how this is working in practice in our biggest customer (the Linux Kernel) as it should be using things like retpolines and needing this code to work.
I also think that this entire pass, while a reasonable thing to have, was a bad idea for retpoline lowering. We found retpolines to be faster than the branch trees created by this in most cases w/ many branches. So this needs a non-trivial update. Not sure who has the time to do this at this point though, I'll ask around w/ folks familiar with this bit of code.
It would be good to at least update the comment to English prose.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2535-2536
+
+ // Retrieve successors. Look through artificial IR level blocks like
+ // catchswitch for successors.
+ MachineBasicBlock *Return = FuncInfo.MBBMap[I.getSuccessor(0)];
----------------
Wait, we handle `catchswitch` combined with this? That seems .... ambitious.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2540-2547
+ // Update successor info.
+ addSuccessorWithProb(CallBrMBB, Return);
+ for (auto *Transfer : Transfers) {
+ MachineBasicBlock *Target = FuncInfo.MBBMap[Transfer];
+ Target->setIsAsmGotoTarget();
+ addSuccessorWithProb(CallBrMBB, Target);
+ }
----------------
Weird that we handle probabilities here but not in Global ISel
================
Comment at: lib/CodeGen/ShrinkWrap.cpp:505-507
+ // Similar to EH handling above, asm-goto targets allow jumping out of the
+ // middle of a basic block.
+ // FIXME: Is this sufficient?
----------------
Wait, they do?
I don't understand -- the callbr is always a terminator. I understand that it may expand to lots of instructions, but isn't that opaque at this level?
================
Comment at: lib/Target/X86/X86AsmPrinter.cpp:256-260
+ case MachineOperand::MO_BlockAddress: {
+ MCSymbol *Sym = P.GetBlockAddressSymbol(MO.getBlockAddress());
+ Sym->print(O, P.MAI);
+ break;
+ }
----------------
The fact that we have to add an entire machine operand layer here makes me feel more strongly that we should just be using the existing support for MBB machine operands rather than this.... But maybe there is a reason I'm just missing so far.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53765/new/
https://reviews.llvm.org/D53765
More information about the llvm-commits
mailing list