Page MenuHomePhabricator

[globalisel][tablegen] Add support for importing 'imm' operands.
ClosedPublic

Authored by dsanders on Jul 25 2017, 5:41 AM.

Details

Summary

This patch enables the import of rules containing 'imm' operands that do not
constrain the acceptable values using predicates. Support for ImmLeaf will
arrive in a later patch.

Depends on D35681

Event Timeline

dsanders created this revision.Jul 25 2017, 5:41 AM
rovka accepted this revision.Jul 27 2017, 4:15 AM

Seems fine.

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
352

You could also assert that the OldInsn is a G_CONSTANT.

353

It would be really easy to also handle .isImm() operands here. They can't appear if you always use MachineIRBuilder, but IIRC the MIRParser sometimes introduces them, so it wouldn't hurt to support them to avoid surprising people when they write tests.

test/CodeGen/AArch64/GlobalISel/select-imm.mir
30

Nit: Maybe test a negative constant instead, to make sure we do the right extension?

This revision is now accepted and ready to land.Jul 27 2017, 4:15 AM
dsanders updated this revision to Diff 108661.Jul 28 2017, 8:51 AM
dsanders marked 3 inline comments as done.

Added assertion.
Added support for isImm() case.
Switched to negative number on the i32 test case.

  • Fixed the issue preventing tablegen from matching either case and killed the C++ that made the test pass.
  • Fixed a priority bug that was revealed after that fix. G_CONSTANT instructions need to be less important than specific constants.
    • I'm thinking that I should replace ConstantIntOperandMatcher with an appropriate instruction/opcode/literal-int matcher combination. I see no good reason for it to be special at this point. I'll leave that for another patch though.
dsanders added inline comments.Jul 28 2017, 8:57 AM
include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
353

Ok.

IIRC the MIRParser sometimes introduces them, so it
wouldn't hurt to support them to avoid surprising people
when they write tests.

You get an isImm() operand when you write '123' instead of 'i32 123'.

test/CodeGen/AArch64/GlobalISel/select-imm.mir
30

Done. This revealed that a small mistake was preventing my code from being executed. The GIM_CheckNumOperands was checking for 1 operand instead of 2 (because Src had no children). The test only passed because the C++ was catching it. I've fixed this and killed the C++ that was handling it. I've also fixed a bug this revealed where ADD8ri had higher priority than INC8r on X86

This also turned up a small inconsistency between the old C++ and tablegen. The C++ uses ZExt but TableGen uses SExt. I haven't decided how to fix this one yet. For AArch64, I should presumably switch to ZExt but this is the wrong thing to do on some targets such as Mips where all register-width integers are signed.

dsanders updated this revision to Diff 108917.Jul 31 2017, 7:02 AM

The C++ uses ZExt but TableGen uses SExt. I haven't decided how to fix this one yet.

I've mostly deferred this problem to a later date but I have made slight tweaks
to make the zero-extension explicit by renaming GIR_CopyConstantAsImm to
GIR_CopyConstantAsUImm. It's up to tablegen to decide whether to sign-extend or
zero-extend the constant but right now it can only choose zero-extend.

rovka added inline comments.Aug 2 2017, 7:22 AM
include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
353

You forgot the == G_CONSTANT

test/CodeGen/AArch64/GlobalISel/select-imm.mir
30

Is there a reason why the old code would ZExt instead of SExt? SExt seems more natural in this context, but I admittedly haven't thought too much about it.

dsanders added inline comments.Aug 2 2017, 8:18 AM
include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
353

Thanks. I guess I was seeing what I expected to see :-)

test/CodeGen/AArch64/GlobalISel/select-imm.mir
30

I'm not sure at the moment. It could be that it makes no difference either way aside from targets like Mips which specifies the value of bits that don't physically exist in an implementation (values are theoretically sign-extended to infinite-width) and relies on this for 32-bit code to work correctly on a 64-bit CPU.

One observation from the patches I've been working on this week is that ImmLeaf's sign-extend the constant internally so it would make sense for the same operator without any predicates (imm) to do the same. Unfortunately, that argument can be used the other way if you start from the PatFrag's which start with an SDNode and frequently zero-extend it.

dsanders updated this revision to Diff 109533.Aug 3 2017, 6:12 AM
dsanders marked 2 inline comments as done.

Fix == TargetOpcode::G_CONSTANT nit.

dsanders added inline comments.Aug 3 2017, 6:13 AM
test/CodeGen/AArch64/GlobalISel/select-imm.mir
30

SelectionDAG doesn't seem to extend at all for imm without predicates (it just morphs the node from ISD::Constant to AArch64::MOVi32imm, leaving the operands untouched) so I think it's safe to say it doesn't matter what we pick here.

Do you have a preference? If not, I'll stick with the zero-extend for now.

rovka added a comment.Aug 3 2017, 6:27 AM

Well, considering that Mips prefers SExt and nothing (that we're aware of) prefers ZExt, it might be better to just use SExt from the start. But if you're feeling lazy ZExt is fine too :)

That makes sense to me, I'll switch it over to sign-extend.

dsanders closed this revision.Aug 8 2017, 3:45 AM