Page MenuHomePhabricator

[GISel]: Refactor MachineIRBuilder to allow passing additional parameters to build Instrs

Authored by aditya_nandakumar on Dec 4 2018, 1:19 PM.



Previously MachineIRBuilder::buildInstr used to accept variadic arguments for sources (which were either unsigned or MachineInstrBuilder). While this worked well in common cases, it doesn't allow us to build instructions that have multiple destinations. Additionally passing in other optional parameters in the end (such as flags) is not possible trivially. Also a trivial call such as

B.buildInstr(Opc, Reg1, Reg2, Reg3)

can be interpreted differently based on the opcode (2defs + 1 src for unmerge vs 1 def + 2srcs).
This patch refactors the buildInstr to

buildInstr(Opc, ArrayRef<DstOps>, ArrayRef<SrcOps>)
  • where DstOps and SrcOps are typed unions that know how to add itself to MachineInstrBuilder.

After this patch, most invocations would look like

B.buildInstr(Opc, {s32, DstReg}, {SrcRegs..., SrcMIBs..});

Now all the other calls (such as buildAdd, buildSub etc) forward to buildInstr. It also makes it possible to build instructions with multiple defs.
Additionally in a subsequent patch, we should make it possible to add flags directly while building instructions.
Additionally, the main buildInstr method is now virtual and other builders now only have to override buildInstr (for say constant folding/cseing) is straightforward.

Diff Detail


Event Timeline

I also added{F7649492}

which is a clang-tidy implementation to automatically add {} to most buildInstr calls.

Also removed some temporary code that snuck in.

Added support for a few other instructions such as Unmerge, Merge, UADDE etc.
Now all instructions which only involve register operands go through buildInstr. So calling buildInstr(ADD, s32, {a, b}) should be equivalent to buildAdd(s32, {a, b});

I also added{F7649492}

which is a clang-tidy implementation to automatically add {} to most buildInstr calls.

Please link to this in the commit message itself so it's easier to find once the change is committed. Also, it would be nice to add some comments in the file on how to build and run it with clang-tidy (even if it's just linking to clang-tidy docs).

This looks like a big improvement to the API overall, thanks for working on it.

You're a bit inconsistent throughout the implementation of MachineIRBuilder on whether you replace the implementations of buildFoo() methods with calls to buildInstr or not - is there a reason not to make them all forward to the generic implementation?


Does this need to be public? I think it's only used internally to DstOp.

Also, this can probably be an enum class easily enough, since you don't convert it to or from an integer type ever.


It would be clearer to do "case DstType::TyRC" here explicitly, since that's the only case the default catches. OTOH, should it even be legal to call this if you have a register class? Maybe it should assert.


Same comments as on DstType


I'd probably do a switch statement here, even though it's more verbose. That way if we ever add another SrcType it will warn if we forget to update it.

volkan added inline comments.Dec 10 2018, 12:48 PM

Could you add support for CmpInst::Predicate as well? It would be easier to build a G_ICMP/G_FCMP using that.

aditya_nandakumar marked 4 inline comments as done.Dec 10 2018, 12:48 PM

All the buildInstr support so far has been for instructions that have register only operands. The ones missing are probably non register operands (such as predicates, immediate, FPConstant etc)


I think the CSEBuilder might need to use it (depending on if it should materialize a copy or not). I haven't implemented that part yet. If it's not needed, as a part of that change, I'll make it private.

I will change it to enum class.

Updated based on Justin/Volkan's feedback. Moved buildI/FCmp as well to buildInstr.

bogner accepted this revision.Dec 10 2018, 2:17 PM
This revision is now accepted and ready to land.Dec 10 2018, 2:17 PM

Added a clang-tidy patch which should automatically update most API calls to the new API. When possible (most trivial cases), it adds {} around sources and destinations.

Pushed in r348815

Thanks for the review.