This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Support physical register inputs in patterns
ClosedPublic

Authored by arsenm on Jul 18 2019, 3:23 PM.

Details

Reviewers
dsanders
aemerson
Summary

The current matchers seem to not like anonymous operands, so just use
the name of the register as a dummy name.

If the same name of the physical register is used, it treats them as
needing to be the same operand. I'm not sure if this is necessarily
useful or makes any sense. It may be a useful behavior for tied
operands?

I'm also not sure if this should verify the register bank of the input
matches the physical register class. This doesn't work on AMDGPU as-is
because it fails to find the registser class for M0 for some reason,
and for wzr on AArch64.

Diff Detail

Event Timeline

arsenm created this revision.Jul 18 2019, 3:23 PM
aemerson added inline comments.Aug 20 2019, 11:23 AM
test/TableGen/gisel-physreg-input.td
75

How can the SPECIAL register be a part of the GPR32 reg class here?

arsenm marked an inline comment as done.Aug 20 2019, 11:33 AM
arsenm added inline comments.
test/TableGen/gisel-physreg-input.td
75

It's not. This is checking what happens if you for some reason name an operand the same thing as a physical register name

aemerson added inline comments.Aug 20 2019, 11:40 AM
test/TableGen/gisel-physreg-input.td
75

Ok, so it looks like it checks if the operands are the same? I think it should be an error instead.

arsenm marked an inline comment as done.Aug 21 2019, 9:15 AM
arsenm added inline comments.
test/TableGen/gisel-physreg-input.td
75

Either way it differs from the current DAG behavior, which does allow this. It didn't seem worth rewriting everything to avoid referring to operands by name

arsenm updated this revision to Diff 216449.Aug 21 2019, 12:56 PM

Avoid using the name of the register. I think this should match the current DAG behavior if the names would collide

dsanders added inline comments.Aug 30 2019, 3:16 PM
test/TableGen/gisel-physreg-input.td
37–38

I'm a bit surprised to see that there's no register bank check here. I would have expected specifying a register to be equivalent to specifying a register class containing a single register. I suppose it doesn't have to but it does mean that we can contradict the register bank selector decisions by inserting cross-bank copies it was unaware of.

49

FWIW, I'd prefer registers to be explicitly named. This mostly because of possibility of accidental cases of the FIXME below but it's also because I have a general preference for only having one way to specify a name.

53–55

I think this is fine so long as the user is choosing the names. I'm not keen on it when a generated name and a chosen name are the same.

utils/TableGen/GlobalISelEmitter.cpp
832

registser -> register

2350

Could you add a description comment to this class?

2548–2550

Yeah, we really ought to compress the table with sleb/uleb where appropriate so that elements have a minimum size of 1 byte. All the common flags would easily fit into 1 byte, and physical register id is at most 3 bytes (when leb encoded) and will often be 1 byte.

3971–3972

I don't think I understand this comment, it doesn't really give enough context to know why the necessity is in doubt or the problems it avoids

arsenm marked an inline comment as done.Aug 31 2019, 4:39 PM
arsenm added inline comments.
test/TableGen/gisel-physreg-input.td
53–55

The new version avoids names for physical registers, this comment is out of date

arsenm updated this revision to Diff 218245.Aug 31 2019, 6:32 PM

Add reg bank check. This requires a new method to get a register class from a physical register. The existing getRegClassForRegister would fail for AMDGPU for two reasons. First it would give up because the types of all classes weren't exactly the same. With that fixed, it would also find a non-allocatable super class used for operand constraints which does not have a meaningful reg bank.

dsanders added inline comments.Sep 5 2019, 5:19 PM
utils/TableGen/GlobalISelEmitter.cpp
832

This typo is still there, I also missed the anonymouss typo on the first read

2350

This still needs a description comment

3971–3972

This is the only bit that's holding me back from giving a conditional LGTM as I don't understand the comment and therefore I'm not sure if it's an issue. Could you elaborate on what the problems are and why the necessity is in doubt?

arsenm updated this revision to Diff 219024.Sep 5 2019, 7:54 PM

Rebase to avoid dependence on D58232, remove comment and fix typos

arsenm marked an inline comment as done.Sep 5 2019, 7:55 PM
arsenm added inline comments.
utils/TableGen/GlobalISelEmitter.cpp
3971–3972

I think this is a leftover from an early iteration. I don't remember why I was trying to remove this, or see why its should be

dsanders accepted this revision.Sep 6 2019, 10:44 AM

LGTM

test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.sendmsghalt.mir
3 ↗(On Diff #219024)

Did you mean to disable this run line?

This revision is now accepted and ready to land.Sep 6 2019, 10:44 AM
arsenm marked an inline comment as done.Sep 6 2019, 11:14 AM
arsenm added inline comments.
test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.sendmsghalt.mir
3 ↗(On Diff #219024)

No, this was working around an update_mir_checks bug where it doesn’t do anything if multiple run lines use the same check prefix

arsenm closed this revision.Sep 6 2019, 1:31 PM

r371253