This is an archive of the discontinued LLVM Phabricator instance.

[GISel]: Don't assert when constraining Registers which are uses if there's no regclass.
ClosedPublic

Authored by aditya_nandakumar on Feb 16 2018, 1:47 PM.

Details

Summary

Currently we assert that only non target specific opcodes can have missing RegisterClass constraints in the MCDesc. The backend can have instructions with register operands but don't have RegisterClass constraints (say using unknown_tuple) in which case the instruction defining the register will constrain it.
Change the assert to only fire if a def has no regclass.

Diff Detail

Repository
rL LLVM

Event Timeline

There isn't an unknown_tuple in tree. I think you're referring to unknown_class and unknown which have only been used by AMDGPU so far. Is that correct?

There isn't an unknown_tuple in tree. I think you're referring to unknown_class and unknown which have only been used by AMDGPU so far. Is that correct?

Thanks Daniel. That's what I meant (clearly I got confused with names).

Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

LGTM.

Any chance you could add a test case?

qcolombet accepted this revision.Feb 26 2018, 9:30 AM
This revision is now accepted and ready to land.Feb 26 2018, 9:30 AM

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

In D43409#1014897, @dsanders wrote:
Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

I think isReg(), MCOI::OPERAND_UNKNOWN, and regclass -1 combine to indicate unknown but I haven't gone through the whole table to double-check. It doesn't look like there's an accessor to detect MCOI::OPERAND_UNKNOWN in any case.

Given that we can't do better, LGTM with a test case and a nit

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

AMDGPU uses it a couple times in their tablegen definitions. If we're importing those rules you can probably use that

lib/CodeGen/GlobalISel/Utils.cpp
62–63

It could be clearer that 'unless they are uses' refers to the operands.

In D43409#1014897, @dsanders wrote:
Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

I think isReg(), MCOI::OPERAND_UNKNOWN, and regclass -1 combine to indicate unknown but I haven't gone through the whole table to double-check. It doesn't look like there's an accessor to detect MCOI::OPERAND_UNKNOWN in any case.

Given that we can't do better, LGTM with a test case and a nit

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

AMDGPU uses it a couple times in their tablegen definitions. If we're importing those rules you can probably use that

I don't think AMDGPU imports any rules from what I see.

In D43409#1014897, @dsanders wrote:
Hmm, I'm a bit reluctant to make this conditional on whether it's a use/def. Doing that feels like a bigger relaxation on the assertion than should be needed. Is it possible to make it conditional on whether the constraint is specified to be unknown?

I don't think the unknown part is available in the InstrDescription - we only see the RegClass as -1 for an instruction which uses unknown operands.

I think isReg(), MCOI::OPERAND_UNKNOWN, and regclass -1 combine to indicate unknown but I haven't gone through the whole table to double-check. It doesn't look like there's an accessor to detect MCOI::OPERAND_UNKNOWN in any case.

Given that we can't do better, LGTM with a test case and a nit

LGTM.

Any chance you could add a test case?

I don't see any upstream GISel target using unknown as operand type and I'm not sure if creating a fake instruction for a target and then selecting it + constraining it is straightforward/necessary.

AMDGPU uses it a couple times in their tablegen definitions. If we're importing those rules you can probably use that

I don't think AMDGPU imports any rules from what I see.

In that case I don't think this is test-able in-tree. LGTM with just the nit

Pushed in 326142