This is an archive of the discontinued LLVM Phabricator instance.

[GISel] Teach TableGen to check predicates of immediate operands in patterns
ClosedPublic

Authored by gargaroff on Nov 18 2020, 6:00 AM.

Diff Detail

Event Timeline

gargaroff created this revision.Nov 18 2020, 6:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 6:00 AM
gargaroff requested review of this revision.Nov 18 2020, 6:00 AM

I haven't worked that much with TableGen, so if this is already supported or if there is a better way to do this, please let me know.

I'm no expert on GlobalISelEmiitter, but it doesn't appear to perform this check now. Your new code looks good to my untrained eye.

Someone else should look at it, too.

arsenm added inline comments.Nov 30 2020, 10:38 AM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
787

Should probably group with case GIM_CheckI64ImmPredicate: {

The name similarity is a bit confusing too

gargaroff added inline comments.Dec 2 2020, 12:00 AM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
787

Do you have a suggestion for a better name? I'm struggling to come up with a different one. The only other idea I have is TImmPredicate, which is just as similar.

foad added a subscriber: foad.Dec 2 2020, 2:46 AM
foad added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
787

"CheckI64ImmOperandPredicate"? Though there are plenty of other checks that check specific operands, that don't have "Operand" in their names.

gargaroff added inline comments.Dec 2 2020, 2:48 AM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
787

I'd be fine with that. @arsenm ?

arsenm added inline comments.Dec 2 2020, 7:40 PM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
787

MachineOperand calls these "isImm" and "isCImm", so these names should probably mirror that. I'm not sure about including "I64"

gargaroff updated this revision to Diff 309831.Dec 7 2020, 1:11 AM
gargaroff marked 3 inline comments as done.

Rebase. Group with GIM_CheckI64ImmPredicate. Change naming to GIM_CheckImmOperandPredicate.

@arsenm I went with @foad's suggestion in the end because it had less impact than trying to make the naming work with CImm and Imm. Also MachineOperand seems to use them in aslightly different meaning, unless I'm misunderstanding. Hope that's fine for you.

dsanders added inline comments.Dec 7 2020, 12:16 PM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
278–281

This assert probably ought to be re-worked a bit as at first glance it appears to be always true. I think it still mostly works but does so indirectly because G_CONSTANT always has an isCImm() (to have type information) and never an isImm().

However, an immediate operand can be isImm() or isCImm() which may give you a false assert for the immediate operands case.

787

I think the main thing is including 'Operand' in the name but I do think the comments need to make the distinction clear. In GlobalISel very few immediates are operands. Normally, the operand is a vreg that's def'd by a G_CONSTANT instruction.

IIRC, the I64 comes from IntImmLeaf isAPInt==0. There was a lot of code that only dealt with <=64-bit and used constants so those convert to int64_t before running the custom C++ predicate. A few needed full APInt's though. I'd be inclined to keep the I64 as sharing code with GIM_CheckI64ImmPredicate means the custom C++ predicate shares the same interface.

MachineOperand calls these "isImm" and "isCImm", so these names should probably mirror that. I'm not sure about including "I64"

isImm() vs isCImm() isn't related to imm/timm. It's about the size of the immediate

/// isImm - Tests if this is a MO_Immediate operand.
/// isCImm - Test if this is a MO_CImmediate operand.
MO_Immediate,         ///< Immediate operand
MO_CImmediate,        ///< Immediate >64bit operand
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

Ah ok, I see why timm is special, it's been special-cased and isn't handled with the other immediates. I'm a little surprised to see this here as this block of code is supposed to be handling MBB's. We should probably revise the comment on 4180 if this is correct. FWIW, I'd generally prefer that we didn't special case by name but I don't really have an alternative to suggest in this case.

What was the reason timm's needed to be handled here rather than with the imm and the other ImmLeaf's? I'd hazard a guess that it's down to the distinction between having an MCOperand with isImm() == true and the normal G_CONSTANT but even if that's the reason, timm vs imm doesn't really correspond to that distinction (timm is just an imm that can't be folded/simplified/lowered) which suggests there's something else not quite right.

gargaroff added inline comments.Dec 15 2020, 3:14 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

Do you have some suggestion what could be done? Even if we remove the special handling here, at one point we will have to do at least some special handling because in one case we have to check for a G_CONSTANT while in the other we have to check for an immediate operand.

I guess the checker in match-table could technically do both dynamically: check one condition first and if that doesn't apply, check the other one. However I'm not sure what changes this would need in this emitter, as the predicate matcher for G_CONSTANT is an InstructionMatcher while for timm we need an OperandMatcher. I'm not too familiar with the internals of TableGen, so the only way to differentiate between the two known to me, is to check by name unfortunately.

At the moment I still cannot find time to work on this, unfortunately. Unless someone has a suggestion for a simple way forward, I'm going to abandon this next week.

dsanders added inline comments.Mar 10 2021, 4:05 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

For the checking by name bit, I was just saying that I don't really like using the name but there's not really a reasonable alternative in this case. We can leave it distinguishing them by name

For the imm/timm not corresponding to G_CONSTANT/isImm() bit, I was mostly acknowledging that something doesn't quite fit with the mapping from DAGISel to GlobalISel in general not really w.r.t this patch. The problem is that the distinction between Constant/TargetConstant is rather artificial, they're both constants that can match and be rendered as isImm() immediates and they can both be materialized in a register. The only distinction is what optimizations DAGISel is allowed to apply to them. I think the closest we can get in GlobalISel is that both imm and timm are allowed to match G_CONSTANT's, but timm is also allowed to match isImm() immediates as well. The one bit that still doesn't quite match up with DAGISel there is if you have an isImm() style immediate but no timm rules that match it, you'll get cannot-select whereas DAGISel would have materialized it.

Does that cover what you need?

arsenm added inline comments.Mar 10 2021, 4:51 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

The problem is that the distinction between Constant/TargetConstant is rather artificial, they're both constants that can match and be rendered as isImm() immediates and they can both be materialized in a register

No, this is the real and significant distinction. Constant is materialized in a register, TargetConstant must not be. Transforming a constant from a materialized one into an isImm operand is a function of the output operand transform

dsanders added inline comments.Mar 10 2021, 5:21 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

There's nothing I've seen in the definition of timm/TargetConstant that forbids materialization in a register. Implementation-wise they're both ConstantSDNode's and are only distinguishable by the opcode being either Constant or TargetConstant. This keeps them apart in the rules as DAGISel rules which check the opcode rather than isa<ConstantSDNode> but aside from having a different enum value in ISD enum they're functionally the same in DAGISel's implementation.

If it's true that timm never materializes for a given target then it's only by a lack of a (set $dst, $src:timm) pattern rather than by anything in the nature of timm itself. Maybe that's common enough that we can get away with enforcing it in the importer but it's not enforced in DAGISel as far as I've seen.

dsanders added inline comments.Mar 10 2021, 5:29 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

If it's true that timm never materializes for a given target then it's only by a lack of a (set $dst, $src:timm) pattern rather than by anything in the nature of timm itself. Maybe that's common enough that we can get away with enforcing it in the importer but it's not enforced in DAGISel as far as I've seen.

Just to clarify the last bit of that, if is true via the lack of a materialization pattern for all targets then I'll be ok with the importer only accepting isImm() immediates for when timm is used. I think we should probably verify the materialization pattern for timm doesn't exist and error if we find it though.

gargaroff added inline comments.Mar 23 2021, 5:57 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

I tried to pick up this patch again but quickly realized that I'm still confused about what actually needs to be done now. Should the code handling timms be moved to some other place where it makes more sense? Should the cases for immediates in the instruction selector be merged? Or should the whole immediate handling be changed in some way?

I have never worked with DAG ISel and I don't know what the difference between imm and timm is supposed to be in the end. In my understanding it was just the distinction that imm maps to G_CONSTANT operand, while timm maps to an immediate operand, nothing more. From your discussion it seems like there is something more to this that would actually require a much bigger rework in this patch. Is that correct?

dsanders added inline comments.Mar 23 2021, 12:28 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

In my understanding it was just the distinction that imm maps to G_CONSTANT operand, while timm maps to an immediate operand, nothing more.

Where I landed on this was while this is technically wrong w.r.t the definition of imm/timm, it's right w.r.t practice. We just need to add a check along the lines of this pseudocode:

for (Pattern : Patterns) {
    if (Pattern.getSrcPattern() is not (set $d, timm))
        report_fatal_error("Materialization of timm cannot work in GISel");
}

to ensure we don't get bitten by the difference between what the definition allowed and what we implemented.

arsenm added inline comments.Mar 23 2021, 12:31 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

Not materializing in a register is the whole point, otherwise we would not have the distinction. I did make the SelectionDAG patterns stricter in this area not too long ago, so I don't think a verifier check would show too many violations

dsanders added inline comments.Mar 23 2021, 1:08 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

Not materializing in a register is the whole point, otherwise we would not have the distinction.

It may be the overall point but there's nothing in the definitions (implementation or documentation) that expresses this limitation. I've come around to the opinion that the limitation is pretty much there in practice but without any docs/implementation I'm nervous about _relying_ on that without some guard against being wrong.

I did make the SelectionDAG patterns stricter in this area not too long ago, so I don't think a verifier check would show too many violations

I'm hoping it will be zero. The reason I'd like the check is that a rule that violates this new limitation won't be considered a bug in GISel's importer, it will be on the target author to change their rules.

gargaroff added inline comments.Mar 29 2021, 7:29 AM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

So I implemented this check now and tried to write a test for it like this:

def Foo : I<(outs GPR32:$dst), (ins i32imm:$src),
  [(set GPR32:$dst, tuimm9:$src)]
>;

However this pattern is already getting rejected during the import:

llvm/test/TableGen/immarg-predicated.td:24:1: warning: Skipped pattern: Pattern operator lacks an equivalent Instruction (ISD::TargetConstant)
def Foo : I<(outs GPR32:$dst), (ins i32imm:$src),
^

Is there something wrong with my test pattern here or is this already handled then, indirectly?

arsenm added inline comments.Mar 29 2021, 12:50 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

The TargetConstant handling shouldn't have reached this point. The predicate code should have special cased the target constants

dsanders added inline comments.Mar 29 2021, 2:10 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
4187

However this pattern is already getting rejected during the import:

llvm/test/TableGen/immarg-predicated.td:24:1: warning: Skipped pattern: Pattern operator lacks an equivalent Instruction (ISD::TargetConstant)
def Foo : I<(outs GPR32:$dst), (ins i32imm:$src),
^
Is there something wrong with my test pattern here or is this already handled then, indirectly?

Ah ok, I didn't realize we didn't have it mapped at all. In that case let's just put a comment next to:

def : GINodeEquiv<G_CONSTANT, imm>;

in SelectionDAGCompat.td saying that timm is intentionally not mapped so the materialization rule can't violate the expected distinction between imm and timm.

Add comment to SelectionDAGCompat.td

Remove unintended change. Rework assertion in InstructionSelectorImpl.h

gargaroff marked 2 inline comments as done.Mar 30 2021, 12:20 AM
Harbormaster completed remote builds in B96253: Diff 334062.
dsanders accepted this revision.Apr 29 2021, 5:50 PM

LGTM. Sorry for the long wait

This revision is now accepted and ready to land.Apr 29 2021, 5:50 PM
This revision was landed with ongoing or failed builds.Apr 30 2021, 1:18 AM
This revision was automatically updated to reflect the committed changes.