This is an archive of the discontinued LLVM Phabricator instance.

[globalisel][tablegen] Add support for COPY_TO_REGCLASS.

Authored by dsanders on May 26 2017, 3:40 AM.



As part of this

  • Emitted instructions now have named MachineInstr variables associated with them. This isn't particularly important yet but it's a small step towards multiple-insn emission.
  • constrainSelectedInstRegOperands() is no longer hardcoded. It's now added as the ConstrainOperandsToDefinitionAction() action. COPY_TO_REGCLASS uses an alternate constraint mechanism ConstrainOperandToRegClassAction() which supports arbitrary constraints such as that defined by COPY_TO_REGCLASS.

Diff Detail


Event Timeline

dsanders created this revision.May 26 2017, 3:40 AM
qcolombet edited edge metadata.Jun 2 2017, 7:04 PM

Hi Daniel,

Looks globally fine. Mainly two remarks:

  • Put more comment on the APIs and classes definition
  • Double check why tablegen gives us gpr32 instead of gpr32all


36 ↗(On Diff #100380)

Add comments on what this is doing.
Don't hesitate to move the comment from the other helper and cross reference to this one.

29 ↗(On Diff #100380)

TargetRegisterClass &

33 ↗(On Diff #100380)

Use TargetRegisterClass &

98 ↗(On Diff #100380)

That change is strange. How does tablegen come up with this constraint?
I would have expected that we get the less constrained regclass (gpr32all in that case)

197 ↗(On Diff #100380)


476 ↗(On Diff #100380)

Remark: Long term we should be able to use the right regbank directly.

1043 ↗(On Diff #100380)

Comment on the class' purpose

1056 ↗(On Diff #100380)


1543 ↗(On Diff #100380)

Could you use explicit types here?

1560 ↗(On Diff #100380)


1568 ↗(On Diff #100380)


qcolombet added inline comments.Jun 2 2017, 7:11 PM
488 ↗(On Diff #100380)

I didn't get that BTW

dsanders updated this revision to Diff 101467.Jun 5 2017, 3:31 PM
dsanders marked 10 inline comments as done.

Fix all the nits.

Only one issue remains. I'll comment on this one in phabricator

The remaining issue I mentioned in the update is this one.

  • Double check why tablegen gives us gpr32 instead of gpr32all

This happens because the SelectionDAG rules specify gpr32. For example:

// (bitconvert:i32 FPR32:f32:$Xn) => (COPY_TO_REGCLASS:i32 FPR32:f32:$Xn, GPR32:i32)

We can probably change the SelectionDAG rules. I'll give this a try

98 ↗(On Diff #100380)

It's explicitly stated to be GPR32 in the SelectionDAG rule. Here's the pattern comment from the imported rules:
// (bitconvert:i32 FPR32:f32:$Xn) => (COPY_TO_REGCLASS:i32 FPR32:f32:$Xn, GPR32:i32)

197 ↗(On Diff #100380)

This one has the same cause:
// (bitconvert:i64 FPR64:f64:$Xn) => (COPY_TO_REGCLASS:i64 FPR64:f64:$Xn, GPR64:i32)

476 ↗(On Diff #100380)

I agree. I manually experimented with this a bit while fixing the compile-time regression. To do it properly I'll need to refactor some of the RegisterBankEmitter to be able to re-use this information from other tablegen emitters.

488 ↗(On Diff #100380)

This is a bad copy/paste. It's referring to something that only matters to the xor rules.

dsanders updated this revision to Diff 101485.Jun 5 2017, 5:27 PM

Update the SelectionDAG rules for FPR->GPR bitcasts to use GPR32all and GPR64all

ab accepted this revision.Jun 15 2017, 2:32 PM

LGTM without the pattern changes.

83 ↗(On Diff #101485)

Comment? Or, alternatively, inline the one-line body in the emitted code?

5588–5592 ↗(On Diff #101485)

I don't think this is correct: FMOV really does take GPR32/GPR64, not GPR32all/GPR64all, and that's what copyPhysReg looks for.

And the c++ selector actually also looks wrong. I'm guessing it happens to work because we never allocate SP, so anything we allocate out of GPR32all is in GPR32 as well anyway.

1046 ↗(On Diff #101485)

Unnecessary 'private', here and below.

This revision is now accepted and ready to land.Jun 15 2017, 2:32 PM
This revision was automatically updated to reflect the committed changes.
dsanders marked 3 inline comments as done.

Made all requested changes in the commit rL305791