@aditya_nandakumar Can you please take a look at this patch? We are unsure on whether the observer is used correctly here.

Bump

Machine operand is now updated in constrainOperandRegClass()

Fixed bad comment

I believe it is calling the one in LowLevelTypeImpl.h.

Fix lint warnings
Add testcase

Bump

Removed use of temporary file.

Moved testcase to correct location
Split RUN into multiple lines
Added check of frame object list
Fixed failing testcase

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).

• Directly apply mapping of there is only one. This leads to improved performance as well as improved code quality for targets that only output a single mapping that is context-dependent (now only failing 32 testcases when forcing greedy to use global instead).
• Bug fix

More bugfixes

Reduce number of necessary checks.

Bug fixes

Use MappingCost instead of int for representing costs.

Bug fix

• Improved selection: Now choosing the mapping with the highest utility instead of lowest cost, which should direct optimization towards blocks with high frequency.
• Reduce vector copying.

Rearranged function declarations in RegBankSelect.cpp (NFC)

Rewrote code to make use of applyMapping(). Now fast, greedy, and global version
all use the same code for enforcing the selection of instruction mappings.

Missed a part of the CHECK

Fixed testcase according to bjope's feedback

Replace dyn_cast with isa

In D90304#2391033, @tschuett wrote:

Sorry for being terse. What I meant to say, you could take an existing test case and show the effects of the different algorithms. There is probably an existing test case for the existing algorithms.

In D90304#2390935, @tschuett wrote:

How about a test-case with fast, greedy, and global to show the differences?

In D90304#2385167, @gargaroff wrote:

I haven't had the time to test this in the new state yet, but I would love to see this integrated. However I'm not in a position to review and approve this. Someone of the GlobalISel folks should do that. As for testing, ideally you could simply leverage the existing tests of the existing GlobalISel backends by enabling this algorithm conditionally. This also requires the approval of those backend maintainers however.

Maybe it would make sense to ping the mailing list again, explaining the current state, giving some numbers how this algorithm impacts compile time and code gen, and try to discuss some integration strategy.

Added Paul-C-Anagnostopoulos since he's now one of the TableGen code owners.

Added Paul-C-Anagnostopoulos since he's now one of the TableGen code owners.

In D86617#2362244, @arsenm wrote:

In D86617#2324324, @ehjogab wrote:

Any comments? Should this patch be accepted or rejected?

I mostly see this as making the behavior more opaque by changing based on the specific register classes involved, so I'm not in favor. What do others think?

So what happens now? Should it be merge as is, or does it need to be tested more stringently before that happens? Or are there any further changes that need to be made?

Bump

replace 'optimal' with 'global'

• fix bug when register is a physical register
• fix lint warnings

Fixed arsenm's comment on MachineIRBuilder

Fixes based on arsenm's comments

Fix lint warnings

Thank you very much for your feedback, arsenm! Really appreciate it!

Any comments? Should this patch be accepted or rejected?

Seems no one else is requesting this.

Bump

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

Bump

Rebased to latest (forgot to pull)

Rebase to latest master

Removed else after return

Oops, had missed that. Thanks!

• add missing check in testcase
• fix lint warnings

In D86617#2238779, @arsenm wrote:

Needs test. I thought I handled this in ee3feef5aaaa3c385fbe08bdb2d48829ad440b56?

Added testcase

In D86203#2294491, @madhur13490 wrote:

Is this still planned to be checked-in?

Reordered functions as per bjope's feedback

Updated patch according to bjope's suggestion.

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

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.

I should mention that this is required for split arguments to work for our target, and it is possible that we're doing something wrong on our end. I don't see it, however, so I decided to submit this for review in hopes that someone either can tell me whether our backend is incorrectly implemented or this is indeed a bug in global-isel.

Forgot to update testcase.

[TableGen][GlobalISel] Fix handling of zero_reg

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.

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.

In D86200#2226354, @arsenm wrote:

In D86200#2226337, @ehjogab wrote:

In D86200#2226077, @arsenm wrote:

I'm curious why you need this. These fields are mostly a hack for SelectionDAG?

In our target argument pointers are converted into MVT::iPTRs, but in light of your comment that might be a bug in our backend. Could you comment on how, and if, MVT::iPTR are suppose to be used in GlobalISel, or are they only intended for SelectionISel?

Ideally MVT wouldn't be used at all, and we're just using the existing SelectionDAG calling convention lowering with its ties to MVT as a crutch. LLT naturally preserves that the value is a pointer with address space and size

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

In D86200#2226077, @arsenm wrote:

I'm curious why you need this. These fields are mostly a hack for SelectionDAG?