[GlobalISel][TableGen] Add handling of unannotated dst pattern ops

[ehjogab]

Summary

GlobalISelEmitter.cpp requires that destination pattern have the
register class annotated to its operands, e.g.:

def : Pat<(add i32:$src1, i32:$str2),
          (instr regclass:$src1, regclass:$src2)>;

This information is not necessary for SelectionIDAG, and subsequently
for certain targets the register class is omitted from the operands,
which causes the pattern to be rejected. However, this information is
still available by looking at the corresponding operand in the
instruction definition. This patch performs such a lookup when a
register class is expected and none has been annotated to the operand.

[arsenm]

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

[ehjogab]

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

[arsenm]

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

[ehjogab]

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem. However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

[madhur13490]

In D86203#2229816, @ehjogab wrote:

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem. However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

When a register class is an union of two or more classes then such problems bound to occur for one or more targets. Why not do such inference when it is obvious i.e. register class in instruction definition is a leaf register class and NOT an union of some base other base classes. I think this is better than having an runtime option to enable/disable the inference.
I think leaf classes can be detected programmatically.

[ehjogab]

In D86203#2230217, @madhur13490 wrote:

In D86203#2229816, @ehjogab wrote:

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem. However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

When a register class is an union of two or more classes then such problems bound to occur for one or more targets. Why not do such inference when it is obvious i.e. register class in instruction definition is a leaf register class and NOT an union of some base other base classes. I think this is better than having an runtime option to enable/disable the inference.
I think leaf classes can be detected programmatically.

I like that idea, and this is only applied on leaf nodes to begin with.

[arsenm]

In D86203#2229816, @ehjogab wrote:

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem.

As I mentioned, the VS_32 is unallocatable and only exists to model the operand constraints. The actual operands should be either VGPR_32 or SGPR_32. Querying the register bank bank for VS_32 doesn't make any sense, and I suspect would assert.

However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

This would produce a broken import by default with unannotated patterns, which is undesirable. This still has the same problem before, but now all the patterns are broken and it's harder to find which ones need fixing.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

I think tablegen is hard enough to understand as is, and changing behaviors based on targets would make that problem worse.

[madhur13490]

In D86203#2230218, @ehjogab wrote:

In D86203#2230217, @madhur13490 wrote:

In D86203#2229816, @ehjogab wrote:

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem. However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

When a register class is an union of two or more classes then such problems bound to occur for one or more targets. Why not do such inference when it is obvious i.e. register class in instruction definition is a leaf register class and NOT an union of some base other base classes. I think this is better than having an runtime option to enable/disable the inference.
I think leaf classes can be detected programmatically.

I like that idea, and this is only applied on leaf nodes to begin with.

If it is applied to leaf nodes in this patch then it shouldn't be a problem to our AMDGPU backend, isn't it @arsenm?

[ehjogab]

In D86203#2230458, @arsenm wrote:

In D86203#2229816, @ehjogab wrote:

In D86203#2226535, @arsenm wrote:

In D86203#2226357, @ehjogab wrote:

In D86203#2226073, @arsenm wrote:

What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities

Hm, not sure what you mean. Could you provide a brief example?

The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either.
e.g.
RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.

In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.

The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem.

As I mentioned, the VS_32 is unallocatable and only exists to model the operand constraints. The actual operands should be either VGPR_32 or SGPR_32. Querying the register bank bank for VS_32 doesn't make any sense, and I suspect would assert.

However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.

This would produce a broken import by default with unannotated patterns, which is undesirable. This still has the same problem before, but now all the patterns are broken and it's harder to find which ones need fixing.

Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?

I think tablegen is hard enough to understand as is, and changing behaviors based on targets would make that problem worse.

If we could make this so that it only applies in situations where what is already annotated to the instruction operand is the only sensable option for the pattern opernads - i.e. only do default annotation when the pattern operand is a leaf and the register class of the instruction operand is (1) allocatable, and (2) not a union of two other register classes - would that be acceptable? I agree that we should not create broken imports nor make things harder to debug, but if we can automate things in a safe manner, why not do it?

[ehjogab]

Added check that the register class to be annotated is allocatable and is not a union of other register classes.

[madhur13490]

Is this still planned to be checked-in?

[ehjogab]

In D86203#2294491, @madhur13490 wrote:

Is this still planned to be checked-in?

Dunno. I got no response on my last edit, and I've been on vacation since then until today so there's been no updates on my side.
I still don't see why this shouldn't be checked in, but I might be overlooking something that speaks against this.

[ehjogab]

Bump

[arsenm]

Does not annotating the output really buy you much? I think inconsistent behaviors in tablegen is a bigger obstacle to understanding patterns

llvm/utils/TableGen/GlobalISelEmitter.cpp

4161–4162

I think trying to parse this manually is problematic

[ehjogab]

In D86203#2303410, @arsenm wrote:

Does not annotating the output really buy you much? I think inconsistent behaviors in tablegen is a bigger obstacle to understanding patterns

It buys me the effort of having to go through all patterns in our target and add these manually. There are over 1300 patterns, meaning it will be tedious and time-consuming having to go through each and every one, look up the register classes of all arguments to the instruction, and annotate these. But since everyone else have had to do this for other targets, I suppose I will be forced to do the same.

Unless someone else is willing to argue towards having this patch accepted, I see no point in spending more time on this.

[bjope]

One thing that I've not understood is how GlobalISel is different from the legacy ISel here. Apparently it isn't necessary to annotate things with the legacy ISel today? So is legacy ISel doing the same thing that this patch suggests to do also for GlobalISel, or why do we suddenly need to update all patterns now (I mean, somehow it has worked fine in the past, right)?

Seems a bit tedious (also from a maintenance perspective) to keep all patterns up-to-date with the instruction definition if the goal is that they should match (which I think has been the case when the annotation has been omitted in the past). If I've for example wanted to restrict an instruction to a more narrow register class, then it's been possible to make a single update in the instruction definition in the past. But now I'd have to find all the patterns using that instruction to also change the annotation? Or maybe it's worse when going in the opposite direction and I want to use a more general register class, then I for sure need to make updates all over the place to make it work.

If annotations are needed, then maybe we need some kind of debug tool to find any inconsistencies? Or is there already an easy way to spot such things?

[ehjogab]

Seems no one else is requesting this.

[foad]

Querying the register bank bank for VS_32 doesn't make any sense, and I suspect would assert.

Would it really be so bad for AMDGPU to define getRegBankFromRegClass(VS_32) == VGPRRegBank in order to get the behaviour it wants from imported patterns?

[bjope]

But if I understand this patch correctly it has tried to derive the type from "some_isnt" when it is omitted, rather than reusing the type from the source pattern. Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

I mean, as far as I can tell the output from the emitter is the same if specifying the register class or just the type from the source pattern.

Example:

def RC : RegisterClass<"MyTarget", [i32], 32, (add R0)>;

// This works with both SelectionDAGISel and GISel:
def : Pat<(add RC:$src1, RC:$src2),
          (some_inst RC:$src1, RC:$src2)>;

// This works with both SelectionDAGISel and GISel:
def : Pat<(add i32:$src1, i32:$src2),
          (some_inst i32:$src1, i32:$src2)>;

// This is currently not supported by GISel:
def : Pat<(add i32:$src1, i32:$src2),
          (some_inst $src1, $src2)>;

// This is currently not supported by GISel:
def : Pat<(add RC:$src1, RC:$src2),
          (some_inst $src1, $src2)>;

[arsenm]

In D86203#2445098, @bjope wrote:

But if I understand this patch correctly it has tried to derive the type from "some_isnt" when it is omitted, rather than reusing the type from the source pattern. Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

The problem isn't the type, it's the register class. It can be ambiguous to go from the target instruction back to the source bank if the target register class supports multiple banks

[bjope]

In D86203#2453302, @arsenm wrote:

In D86203#2445098, @bjope wrote:

But if I understand this patch correctly it has tried to derive the type from "some_isnt" when it is omitted, rather than reusing the type from the source pattern. Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

The problem isn't the type, it's the register class. It can be ambiguous to go from the target instruction back to the source bank if the target register class supports multiple banks

I assume tblgen will complain if it find something ambigous?

However, this pattern seem to work with both SelectionDAGISel and GISel

def : Pat<(add i32:$src1, i32:$src2),
          (some_inst i32:$src1, i32:$src2)>;

and there aren't even any register class annotations (that is why I don't quite get how the register class is the problem).
Also, I could not find any differences in the GenGlobalISel.inc file is using a registerclass instead of "i32" (e.g. in the output part of the pattern), so it did not seem to matter really (unless some_inst was INSERT_SUBREG or EXTRACT_SUBREG or similar that actually might depend on the register class).

I still haven't figured out the use-case when I want/need to use different annotations for an operand in the input pattern compared to the output pattern (is that perhaps an "implicit" COPY_TO_REGCLASS?). It really feels a bit unexpected/unintuitive that for example $src1 could have different types (or register classes if you prefer) in the input and output pattern fragments (isn't it a weird syntax?).

My idea was simply to support an omitted annotation in the output part of the pattern, by defining it as having the same annotation as in the input pattern if it is omitted. After all, it is allowed to omit the annotation today (but then the GlobalISelEmitter ignores the pattern so it isn't really that useful). I think that either we should require an annotation also in the output pattern, or we should support omitting it in some useful way.

Defining what an omitted annotation means does not prevent anyone from adding annotations when they are needed (to resolve ambiguity etc).

[ehjogab]

Hm, I had another look at this and it seems that it's enough to provide either a register class or a value type. In both cases you enter the same if condition (see line 4266 in GlobalISelEmitter.cpp in this review), which checks that the child record is either a RegisterClass, RegisterOperand, or ValueType. If the check passes, it will output a GIR_COPY, which just copies the operand from one instruction (the one matched) to another (the instruction to emit) and hence does not look at the register class of the destination (that seems to be taken care of at a later stage, which does not look at the annotation of the operand in the pattern).

In other words, as long as the operand in the destination pattern is annotated with either register class or register value, TableGen will not reject the pattern. In light of this, maybe it's a better idea to look for the corresponding operand in the source pattern and try to copy its annotation (like bjope suggested).

[dsanders]

But since everyone else have had to do this for other targets, I suppose I will be forced to do the same.

I think it was mandatory to have them at one point and many targets have it because of that. This might be the first time I've seen a rule omit it.

One thing that I've not understood is how GlobalISel is different from the legacy ISel here. Apparently it isn't necessary to annotate things with the legacy ISel today? So is legacy ISel doing the same thing that this patch suggests to do also for GlobalISel, or why do we suddenly need to update all patterns now (I mean, somehow it has worked fine in the past, right)?

The semantics for DAGISel are complicated and largely undocumented. There's quite a few features that only come up once or twice or where a small change can make a pattern more conventional and remove the need to implement something. The goal wasn't really to re-implement the whole thing but rather to aid bring up and eventually transition to GlobalISel.

The big difference that matters w.r.t this though is that GlobalISel has a concept of register banks that didn't exist in DAGISel. The matcher side treats ValueType as a wildcard for RegisterBanks that matches anything. That can be a problem for some targets and such targets need to avoid using it but for others it can be fine. The renderer will copy the operand and thereby preserve bank information that already exists but something (typically GIR_ConstrainSelectedInstOperands ) will have to constrain it to a class for ISel to have done its job.

If annotations are needed, then maybe we need some kind of debug tool to find any inconsistencies? Or is there already an easy way to spot such things?

CodeGenDAGPatterns should object if the source and dest patterns disagree

I assume tblgen will complain if it find something ambigous?

Maybe. It will try for some things but I wouldn't rely on that to be complete.

Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

I think that's probably the right semantics to match DAGISel and if it is, then I feel that it ought to be CodeGenDAGPatterns's job to fill in the blanks. Have you found DAGISel's code for it?

But if I understand this patch correctly it has tried to derive the type from "some_isnt" when it is omitted, rather than reusing the type from the source pattern. Would it make more sense to infer the type from the source pattern instead of from the target instruction when it is omitted? Is that possible?

The problem isn't the type, it's the register class. It can be ambiguous to go from the target instruction back to the source bank if the target register class supports multiple banks

I think you may be talking cross-purposes here. It seems to me that there's two problems being discussed:

• What does the rule mean? I think filling in the blanks in the dst pattern using the values in the src pattern is the right semantics but I haven't dug into DAGISel to confirm that
• Is the resulting rule valid/importable? The answer will be target dependent according to the class->bank mappings. If it's unambiguous then I think accepting it is fine but if it's ambiguous (either by a single ambiguous class->multiple bank mapping, or by type->multiple class->multiple bank mapping) then we should reject the rule for that reason. That information should exist in RegisterBankEmitter.cpp, we might need to make it available to GlobalISelEmitter.cpp somehow

llvm/utils/TableGen/GlobalISelEmitter.cpp

4161–4162

I agree. That should be left to SetTheory, we should only be accessing register classes via CodeGenRegisterClass in this code

4281

I expect CodeGenRegisterClass has something for this. I haven't checked though

llvm/test/TableGen/GlobalISelEmitter-unannotated-dst-pattern-ops.td

+// RUN: llvm-tblgen %s -gen-global-isel -optimize-match-table=true -I %p/../../include -I %p/Common -o - | FileCheck %s
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+def ADD : I<(outs GPR32:$dst), (ins GPR32:$src1, GPR32:$src2), []>;
+
+def X0 : Register<"x0"> { let Namespace = "MyTarget"; }
+def X32 : RegisterClass<"MyTarget", [i32], 32, (add X0)>;
+
+let isAllocatable = 0 in
+def UnallocRC : RegisterClass<"MyTarget", [i32], 32, (add R0)>;
+def UnionRC : RegisterClass<"MyTarget", [i32], 32, (add GPR32, X32)>;
+
+def ADDUnallocRC : I<(outs UnallocRC:$dst),
+                     (ins UnallocRC:$src1, UnallocRC:$src2),
+                     []>;
+def ADDUnionRC : I<(outs UnionRC:$dst), (ins UnionRC:$src1, UnionRC:$src2),
+                   []>;
+
+// CHECK: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_ADD,
+// CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s32,
+// CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
+// CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT: // (add:{ *:[i32] } i32:{ *:[i32] }:$src1, i32:{ *:[i32] }:$src2)  =>  (ADD:{ *:[i32] } ?:{ *:[i32] }:$src1, ?:{ *:[i32] }:$src2)
+// CHECK-NEXT: GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/0, /*Opcode*/MyTarget::ADD,
+// CHECK-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT: // GIR_Coverage, 0,
+// CHECK-NEXT: GIR_Done,
+def : Pat<(add i32:$src1, i32:$src2),
+          (ADD $src1, $src2)>;
+
+//// This should be rejected
+//// CHECK-NOT: ADDUnallocRC
+def : Pat<(add i32:$src1, i32:$src2),
+          (ADDUnallocRC $src1, $src2)>;
+
+// This should be rejected
+// CHECK-NOT: ADDUnionRC
+def : Pat<(add i32:$src1, i32:$src2),
+          (ADDUnionRC $src1, $src2)>;

llvm/utils/TableGen/GlobalISelEmitter.cpp

 //===- GlobalISelEmitter.cpp - Generate an instruction selector -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
private:
 
   Expected<action_iterator>
   importExplicitUseRenderers(action_iterator InsertPt, RuleMatcher &M,
                              BuildMIAction &DstMIBuilder,
                              const llvm::TreePatternNode *Dst);
   Expected<action_iterator>
   importExplicitUseRenderer(action_iterator InsertPt, RuleMatcher &Rule,
                             BuildMIAction &DstMIBuilder,
-                            TreePatternNode *DstChild);
+                            TreePatternNode *DstChild,
+                            Record *InstrOpNodeRec);
   Error importDefaultOperandRenderers(action_iterator InsertPt, RuleMatcher &M,
                                       BuildMIAction &DstMIBuilder,
                                       DagInit *DefaultOps) const;
   Error
   importImplicitDefRenderers(BuildMIAction &DstMIBuilder,
                              const std::vector<Record *> &ImplicitDefs) const;
 
   void emitCxxPredicateFns(raw_ostream &OS, StringRef CodeFieldName,
private:
   inferSubRegIndexForNode(TreePatternNode *SubRegIdxNode);
 
   /// Infer a CodeGenRegisterClass which suppoorts \p Ty and \p SubRegIdxNode.
   /// Return None if no such class exists.
   Optional<const CodeGenRegisterClass *>
   inferSuperRegisterClass(const TypeSetByHwMode &Ty,
                           TreePatternNode *SubRegIdxNode);
 
   /// Return the CodeGenRegisterClass associated with \p Leaf if it has one.
   Optional<const CodeGenRegisterClass *>
   getRegClassFromLeaf(TreePatternNode *Leaf);
 
   /// Return a CodeGenRegisterClass for \p N if one can be found. Return None
   /// otherwise.
   Optional<const CodeGenRegisterClass *>
   inferRegClassFromPattern(TreePatternNode *N);
 
if (auto *ChildDefInit = dyn_cast<DefInit>(SrcChild->getLeafValue())) {
 
     return failedImport(
         "Src pattern child def is an unsupported tablegen class");
   }
 
   return failedImport("Src pattern child is an unsupported kind");
 }
 
+static unsigned
+getNumRegClassesInDag(const DagInit *Dag) {
+  std::string Op = Dag->getOperator()->getAsString();
+  if (Op.compare("add") == 0) {
+    unsigned Count = 0;
+    for (auto *Arg : Dag->getArgs()) {
+      if (getInitValueAsRegClass(Arg)) {
+        Count++;
+      }
+      else if (auto *ArgDag = dyn_cast<DagInit>(Arg)) {
+        Count += getNumRegClassesInDag(ArgDag);
+      }
+    }
+
+    return Count;
+  }
+  else if (Op.compare("sequence") == 0) {
+    return 0;
+  }
+  else {
+    llvm_unreachable("Unknown DAG operator");
+  }
+}
+
 Expected<action_iterator> GlobalISelEmitter::importExplicitUseRenderer(
     action_iterator InsertPt, RuleMatcher &Rule, BuildMIAction &DstMIBuilder,
-    TreePatternNode *DstChild) {
+    TreePatternNode *DstChild, Record *InstrOpNodeRec) {
 
   const auto &SubOperand = Rule.getComplexSubOperand(DstChild->getName());
   if (SubOperand.hasValue()) {
     DstMIBuilder.addRenderer<RenderComplexPatternOperand>(
         *std::get<0>(*SubOperand), DstChild->getName(),
         std::get<1>(*SubOperand), std::get<2>(*SubOperand));
     return InsertPt;
   }
if (!DstChild->isLeaf()) {
     if (DstChild->getOperator()->isSubClassOf("SDNode")) {
       auto &ChildSDNI = CGP.getSDNodeInfo(DstChild->getOperator());
       if (ChildSDNI.getSDClassName() == "BasicBlockSDNode") {
         DstMIBuilder.addRenderer<CopyRenderer>(DstChild->getName());
         return InsertPt;
       }
     }
 
     // Similarly, imm is an operator in TreePatternNode's view but must be
     // rendered as operands.
     // FIXME: The target should be able to choose sign-extended when appropriate
     //        (e.g. on Mips).
     if (DstChild->getOperator()->getName() == "timm") {
       DstMIBuilder.addRenderer<CopyRenderer>(DstChild->getName());
       return InsertPt;
     } else if (DstChild->getOperator()->getName() == "imm") {
       DstMIBuilder.addRenderer<CopyConstantAsImmRenderer>(DstChild->getName());
       return InsertPt;
     } else if (DstChild->getOperator()->getName() == "fpimm") {
       DstMIBuilder.addRenderer<CopyFConstantAsFPImmRenderer>(
           DstChild->getName());
       return InsertPt;
     }
 
     if (DstChild->getOperator()->isSubClassOf("Instruction")) {
       auto OpTy = getInstResultType(DstChild);
       if (!OpTy)
         return OpTy.takeError();
 
       unsigned TempRegID = Rule.allocateTempRegID();
       InsertPt = Rule.insertAction<MakeTempRegisterAction>(
           InsertPt, *OpTy, TempRegID);
       DstMIBuilder.addRenderer<TempRegRenderer>(TempRegID);
 
       auto InsertPtOrError = createAndImportSubInstructionRenderer(
Expected<action_iterator> GlobalISelEmitter::importExplicitUseRenderer(
   // that immediate.
   if (const IntInit *ChildIntInit =
           dyn_cast<IntInit>(DstChild->getLeafValue())) {
     DstMIBuilder.addRenderer<ImmRenderer>(ChildIntInit->getValue());
     return InsertPt;
   }
 
   // Otherwise, we're looking for a bog-standard RegisterClass operand.
-  if (auto *ChildDefInit = dyn_cast<DefInit>(DstChild->getLeafValue())) {
-    auto *ChildRec = ChildDefInit->getDef();
+  Record *ChildRec = nullptr;
+  if (auto *ChildDefInit = dyn_cast<DefInit>(DstChild->getLeafValue()))
+    ChildRec = ChildDefInit->getDef();
+  else if (dyn_cast<UnsetInit>(DstChild->getLeafValue())) {
+    // Operand has uninitialized leaf value (meaning the operand has not been
+    // prefixed with anything), so try getting info from corresponding
+    // instruction operand (if provided).
+    if (InstrOpNodeRec) {
+      // If the record is a register class, check that it represents an
+      // allocatable class and is not a union of other register classes
+      if (InstrOpNodeRec->isSubClassOf("RegisterClass")) {
+        if (InstrOpNodeRec->getValue("isAllocatable") &&
+            InstrOpNodeRec->getValueAsBit("isAllocatable") == 0) {
+          return failedImport(
+              "Dst pattern child trying to use an unallocatable " \
+              "register class");
+        }
+        if (InstrOpNodeRec->getValue("MemberList") &&
+            getNumRegClassesInDag(
+              InstrOpNodeRec->getValueAsDag("MemberList")) > 1) {
+          return failedImport(
+              "Dst pattern child trying to use an union register class ");
+        }
+      }
 
+      ChildRec = InstrOpNodeRec;
+    }
+  }
+  if (ChildRec) {
     ArrayRef<TypeSetByHwMode> ChildTypes = DstChild->getExtTypes();
     if (ChildTypes.size() != 1)
       return failedImport("Dst pattern child has multiple results");
 
     Optional<LLTCodeGen> OpTyOrNone = None;
     if (ChildTypes.front().isMachineValueType())
       OpTyOrNone = MVTToLLT(ChildTypes.front().getMachineValueType().SimpleTy);
     if (!OpTyOrNone)
if (ChildRec) {
     if (ChildRec->isSubClassOf("ComplexPattern")) {
       const auto &ComplexPattern = ComplexPatternEquivs.find(ChildRec);
       if (ComplexPattern == ComplexPatternEquivs.end())
         return failedImport(
             "SelectionDAG ComplexPattern not mapped to GlobalISel");
 
       const OperandMatcher &OM = Rule.getOperandMatcher(DstChild->getName());
       DstMIBuilder.addRenderer<RenderComplexPatternOperand>(
           *ComplexPattern->second, DstChild->getName(),
           OM.getAllocatedTemporariesBaseID());
       return InsertPt;
     }
 
     return failedImport(
         "Dst pattern child def is an unsupported tablegen class");
   }
 
   return failedImport("Dst pattern child is an unsupported kind");
 }
 
 Expected<BuildMIAction &> GlobalISelEmitter::createAndImportInstructionRenderer(
     RuleMatcher &M, InstructionMatcher &InsnMatcher, const TreePatternNode *Src,
     const TreePatternNode *Dst) {
   auto InsertPtOrError = createInstructionRenderer(M.actions_end(), M, Dst);
   if (auto Error = InsertPtOrError.takeError())
     return std::move(Error);
 
   action_iterator InsertPt = InsertPtOrError.get();
   BuildMIAction &DstMIBuilder = *static_cast<BuildMIAction *>(InsertPt->get());
 
   for (auto PhysInput : InsnMatcher.getPhysRegInputs()) {
     InsertPt = M.insertAction<BuildMIAction>(
for (unsigned I = 1; I != ExpectedDstINumUses; I += 2) {
       TreePatternNode *ValChild = Dst->getChild(I);
       TreePatternNode *SubRegChild = Dst->getChild(I + 1);
 
       if (DefInit *SubRegInit =
               dyn_cast<DefInit>(SubRegChild->getLeafValue())) {
         CodeGenSubRegIndex *SubIdx = CGRegs.getSubRegIdx(SubRegInit->getDef());
 
         auto InsertPtOrError =
-            importExplicitUseRenderer(InsertPt, M, DstMIBuilder, ValChild);
+            importExplicitUseRenderer(InsertPt, M, DstMIBuilder, ValChild,
+                                      nullptr);
         if (auto Error = InsertPtOrError.takeError())
           return std::move(Error);
         InsertPt = InsertPtOrError.get();
         DstMIBuilder.addRenderer<SubRegIndexRenderer>(SubIdx);
       }
     }
 
     return InsertPt;
if (CGP.operandHasDefault(OperandNode) &&
       const CGIOperandList::OperandInfo &DstIOperand = DstI->Operands[InstOpNo];
       DagInit *DefaultOps = DstIOperand.Rec->getValueAsDag("DefaultOps");
       if (auto Error = importDefaultOperandRenderers(
             InsertPt, M, DstMIBuilder, DefaultOps))
         return std::move(Error);
       ++NumDefaultOps;
       continue;
     }
 
     auto InsertPtOrError = importExplicitUseRenderer(InsertPt, M, DstMIBuilder,
-                                                     Dst->getChild(Child));
+                                                     Dst->getChild(Child),
+                                                     OperandNode);
     if (auto Error = InsertPtOrError.takeError())
       return std::move(Error);
     InsertPt = InsertPtOrError.get();
     ++Child;
   }
 
   if (NumDefaultOps + ExpectedDstINumUses != DstINumUses)
     return failedImport("Expected " + llvm::to_string(DstINumUses) +
for (const auto *DefaultOp : DefaultOps->getArgs()) {
     return failedImport("Could not add default op");
   }
 
   return Error::success();
 }
 
 Error GlobalISelEmitter::importImplicitDefRenderers(
     BuildMIAction &DstMIBuilder,
     const std::vector<Record *> &ImplicitDefs) const {
   if (!ImplicitDefs.empty())
     return failedImport("Pattern defines a physical register");
   return Error::success();
 }
 
 Optional<const CodeGenRegisterClass *>
 GlobalISelEmitter::getRegClassFromLeaf(TreePatternNode *Leaf) {
   assert(Leaf && "Expected node?");