Page MenuHomePhabricator

[MIParser] Set RegClassOrRegBank during instruction parsing
ClosedPublic

Authored by Petar.Avramovic on Oct 14 2019, 9:01 AM.

Details

Summary

MachineRegisterInfo::createGenericVirtualRegister sets
RegClassOrRegBank to static_cast<RegisterBank *>(nullptr).
MIParser on the other hand doesn't. When we attempt to constrain
Register Class on such VReg, additional COPY is generated.
This way we avoid COPY instructions showing in test that have MIR
input while they are not present with llvm-ir input that was used
to create given MIR for a -run-pass test.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2019, 9:01 AM

Motivation is having same behavior for GlobalISel legalizer tests for -stop-after=legalizer (llvm-ir input) and -run-pass=legalizer (mir input created from output of same llvm-ir input by pass before legalizer), later has few extra COPY instructions.
Mips has builtin functions (not available as generic opcode) that correspond to a machine instruction, and as such can be selected during legalization.
When selected, register classes also have to be constrained because other passes leave target specific instructions as they are.
Such builtin functions turned to intrinsic and are handled in MipsLegalizerInfo::legalizeIntrinsic.

arsenm added a subscriber: arsenm.Oct 14 2019, 9:23 AM

Should have dedicated parser test

Petar.Avramovic added a reviewer: arsenm.

Add dedicated test for MIParser.

arsenm added inline comments.Tue, Oct 15, 11:01 AM
test/CodeGen/MIR/Mips/setRegClassOrRegBank.mir
2 ↗(On Diff #225022)

The parser test shouldn't be running the legalizer? -O0 and -global-isel are also unnecessary

Petar.Avramovic marked an inline comment as done.Wed, Oct 16, 3:01 AM
Petar.Avramovic added inline comments.
test/CodeGen/MIR/Mips/setRegClassOrRegBank.mir
2 ↗(On Diff #225022)

The change affects internal state of MachineRegisterInfo and is not visible if we only do the parsing ( -run-pass=none ), but is visible when we try to constrainSelectedInstRegOperands.
I saw a few tests with -run-pass other then none, e.g.
test/CodeGen/MIR/AArch64/print-parse-verify-failedISel-property.mir
test/CodeGen/MIR/AArch64/print-parse-overloaded-intrinsics.mir.
Any suggestions how to make better test?

I'm still not sure I mechanically follow how this ends up causing a problem? Surely the VRegInfo field was zero initialized already?

RegClassOrRegBank is a PointerUnion<const TargetRegisterClass *, const RegisterBank *>.
Plain nullptr is a const TargetRegisterClass * (the first template type)
but static_cast<RegisterBank *>(nullptr) is nullptr of const RegisterBank * type.

file: lib/CodeGen/GlobalISel/RegisterBankInfo.cpp in ...RegisterBankInfo::constrainGenericRegister(...
line :
if (RegClassOrBank.is<const TargetRegisterClass *>())
makes the difference.

if we go through IRTranslator, it uses createGenericVirtualRegister which also sets RegClassOrRegBank to nullptr RegisterBank *

file: lib/CodeGen/MachineRegisterInfo.cpp in ...MachineRegisterInfo::createGenericVirtualRegister(...

// FIXME: Should we use a dummy register class?
VRegInfo[Reg].first = static_cast<RegisterBank *>(nullptr);
setType(Reg, Ty);
arsenm accepted this revision.Mon, Oct 21, 10:38 AM

LGTM. I think the "dummy register class" FIXME is a pretty definite no

This revision is now accepted and ready to land.Mon, Oct 21, 10:38 AM
This revision was automatically updated to reflect the committed changes.