This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][TableGen] Take first result for multi-output instructions
Needs ReviewPublic

Authored by bjope on Aug 26 2020, 5:52 AM.

Details

Summary

Previously, tblgen would reject patterns where one of its nested
instructions produced more than one result. These arise when the
instruction definition contains 'outs' as well as 'Defs'. This patch
fixes that by always taking the first result, which is how these
situations are handled in SelectionIDAG.

Diff Detail

Event Timeline

ehjogab created this revision.Aug 26 2020, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 5:52 AM
Herald added a subscriber: rovka. · View Herald Transcript
ehjogab requested review of this revision.Aug 26 2020, 5:52 AM

Needs test. I thought I handled this in ee3feef5aaaa3c385fbe08bdb2d48829ad440b56?

ehjogab updated this revision to Diff 294937.Sep 29 2020, 4:53 AM

Added testcase

ehjogab added a comment.EditedSep 29 2020, 4:54 AM

Needs test. I thought I handled this in ee3feef5aaaa3c385fbe08bdb2d48829ad440b56?

It seems that patch does not handle cases of nested instructions, which is the case in our target where this fix is needed.

I've added a testcase now, as requested.

Any comments? Should this patch be accepted or rejected?

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?

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?

I'd be happy to apply a better solution to this problem if you have one.

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

I'm not yet familiar enough with instruction selection, so I'll leave the decision to others.

foad added a subscriber: foad.Jan 25 2022, 2:43 AM

Reverse ping!

This is definitely useful for enabling more AMDGPU patterns to be imported.

@arsenm wrote:

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.

I don't understand this objection. How does it depend on specific register classes? How is this patch morally different from ee3feef5aaaa which did the same thing at the top level of a pattern?

Summary:

situations are handled in SelectionIDAG.

Typo "SelectionDAG"

llvm/utils/TableGen/GlobalISelEmitter.cpp
3474

Typo "SelectionDAG"

bjope added a comment.Jan 25 2022, 3:12 AM

@foad : Just for info, unfortunately @ehjogab had to move on to another project (after having spent some time on migrating our downstream target to support GlobalISel).
I do not exactly remember the details about this particular patch (but it is included in our downstream branch). Anyway, I'd be happy to commandeer this patch and fix things such as the typo, if the solution can be accepted.

Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 4:03 PM

Long term I assume we should just have MIR patterns working for ISel, but as long as we rely on DAG patterns I believe this is a good change (we inevitably need to make some tradeoffs in this case anyway), can we land it or are there still some objections?

Long term I assume we should just have MIR patterns working for ISel, but as long as we rely on DAG patterns I believe this is a good change (we inevitably need to make some tradeoffs in this case anyway), can we land it or are there still some objections?

As far as I'm concerned, this is a reasonable default behavior, though we should probably emit a "warning" when the tablegen debug output is ON.

That said, @arsenm had objections, let's check with him.

bjope commandeered this revision.Nov 3 2023, 5:09 AM
bjope edited reviewers, added: ehjogab; removed: bjope.
bjope updated this revision to Diff 557996.Nov 3 2023, 5:17 AM

Rebased.
Fixed spelling errors.
Moved the new test case into test/TableGen/GlobalISelEmitter-multiple-output.td instead of adding a new test/TableGen/GlobalISelEmitter-multi-output.td file as in earlier diff.

bjope added inline comments.Nov 3 2023, 5:25 AM
llvm/test/TableGen/GlobalISelEmitter-multiple-output.td
94 ↗(On Diff #557996)

If we change this into an instruction with two explicit outs it would still pass.
While I think that it is good to allow having one explicit def and additional implicit-defs, it seems a bit weird to allow multiple explicit outs. Right?

I guess the intent with this patch was to handle the case with implicit defs. But then we still want it to fail when having multiple explicit outs.
I'm not sure yet how to deal with that.

bjope added inline comments.Nov 3 2023, 6:04 AM
llvm/test/TableGen/GlobalISelEmitter-multiple-output.td
94 ↗(On Diff #557996)

Maybe we can do something like this to check the number of explicit defs:

-static Expected<LLTCodeGen> getInstResultType(const TreePatternNode *Dst) {
+static Expected<LLTCodeGen> getInstResultType(const TreePatternNode *Dst,
+                                              const CodeGenTarget &Target) {
+  assert(Dst->getOperator()->isSubClassOf("Instruction"));
+  CodeGenInstruction &InstInfo = Target.getInstruction(Dst->getOperator());
+
+  if (InstInfo.Operands.NumDefs != 1)
+    return failedImport("Dst pattern child is not having single result");
+
   ArrayRef<TypeSetByHwMode> ChildTypes = Dst->getExtTypes();

I'll continue experimenting with that a bit before updating this patch.

bjope updated this revision to Diff 557999.Nov 3 2023, 8:21 AM
bjope marked an inline comment as done.Nov 3 2023, 8:22 AM
bjope added a comment.Nov 8 2023, 8:23 AM

@foad : With the rebased/updated patch this actually show that more AMDGPU patterns can be imported for GISel. So I guess this still is of interest.
@arsenm : Is this OK now when still giving errors when an instruction has multiple explicit defs?

bjope added a comment.Wed, Nov 15, 6:57 AM

Gentle ping!

bjope edited reviewers, added: foad; removed: Paul-C-Anagnostopoulos, ehjogab.Tue, Nov 28, 1:26 AM

Ping (again).

@Pierre-vh / @foad , can you perhaps have a look from the AMDGPU perspective? It sounded like you weren't against this earlier but now after my latest changes it also show the impact on AMDGPU lit tests.

It also sounded earlier that @qcolombet wasn't against this (but wanted to let @arsenm to chime in due to earlier objections). But that was over 2 months ago and still no comments, so I'd be happy if anyone from AMD could have a look.

foad added a comment.Tue, Dec 5, 7:10 AM

@Pierre-vh / @foad , can you perhaps have a look from the AMDGPU perspective? It sounded like you weren't against this earlier but now after my latest changes it also show the impact on AMDGPU lit tests.

So the test diffs are due to some things being selected with newly imported tablegen patterns, instead of with C++ selection code? It's not clear to me exactly which patterns are now being imported.

Anyway the diffs look OK but I think we would want to follow up by seeing if we can improve some of the patterns, and/or remove some redundant C++.

llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-zext.mir
109 ↗(On Diff #557999)

Using S_BFE_U64 seems better here. Perhaps we can regain that by writing a better pattern.

bjope added a comment.Tue, Dec 5, 1:31 PM

So the test diffs are due to some things being selected with newly imported tablegen patterns, instead of with C++ selection code? It's not clear to me exactly which patterns are now being imported.

I guess one can run llvm-tblgen -gen-global-isel -warn-on-skipped-patterns lib/Target/AMDGPU/AMDGPU.td -I include/ -I lib/Target/AMDGPU/ with and without this patch to compare warning about patterns that can not be imported.
Such a comparison shows the following diffs regarding skipped patterns:

< lib/Target/AMDGPU/SOPInstructions.td:1587:1: warning: Skipped pattern: Dst pattern child has multiple results
< lib/Target/AMDGPU/SOPInstructions.td:1689:1: warning: Skipped pattern: Dst pattern child has multiple results
< lib/Target/AMDGPU/SOPInstructions.td:1696:1: warning: Skipped pattern: Dst pattern child has multiple results
< lib/Target/AMDGPU/SIInstructions.td:1752:1: warning: Skipped pattern: Dst pattern child has multiple results
> lib/Target/AMDGPU/SIInstructions.td:1752:1: warning: Skipped pattern: EXTRACT_SUBREG child #0 could not be coerced to a register class
< lib/Target/AMDGPU/SIInstructions.td:1763:1: warning: Skipped pattern: Dst pattern child has multiple results
> lib/Target/AMDGPU/SIInstructions.td:1763:1: warning: Skipped pattern: EXTRACT_SUBREG child #0 could not be coerced to a register class
< lib/Target/AMDGPU/SIInstructions.td:1774:1: warning: Skipped pattern: Dst pattern child has multiple results
> lib/Target/AMDGPU/SIInstructions.td:1774:1: warning: Skipped pattern: EXTRACT_SUBREG child #0 could not be coerced to a register class
< lib/Target/AMDGPU/SIInstructions.td:1786:1: warning: Skipped pattern: Dst pattern child has multiple results
< lib/Target/AMDGPU/SIInstructions.td:1796:1: warning: Skipped pattern: Dst pattern child has multiple results
< lib/Target/AMDGPU/SIInstructions.td:1806:1: warning: Skipped pattern: Dst pattern child has multiple results
< lib/Target/AMDGPU/SIInstructions.td:2350:1: warning: Skipped pattern: Dst pattern child has multiple results