[RISCV][GISEL] Fix legalization for G_MERGE/UNMERGE_VALUES

[michaelmaitland]

There are two bugs fixed by this patch:

1. G_MERGE_VALUES is always true, causing IdxZeroTy and IdxOneTy to always be s64 and s32 respectively.
2. The legalFor check was for the type in index 0, instead of for index 0 and index 1, so we were allowing s64 or s32 in type 0 to be legal for both G_MERGE_VALUES and G_UNMERGE_VALUES.

The existing test was passing because (2) covered up (1).

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-risc-v

Author: Michael Maitland (michaelmaitland)

Changes

There are two bugs fixed by this patch:

1. G_MERGE_VALUES is always true, causing IdxZeroTy and IdxOneTy to always be s64 and s32 respectively.
2. The legalFor check was for the type in index 0, instead of for index 0 and index 1, so we were allowing s64 or s32 in type 0 to be legal for both G_MERGE_VALUES and G_UNMERGE_VALUES.

The existing test was passing because (2) covered up (1).

---

Full diff: https://github.com/llvm/llvm-project/pull/75619.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp (+7-5)

diff --git a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
index 8f03a7ac41d37b..b46269fb9565fb 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
@@ -85,14 +85,16 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST)
 
   // Merge/Unmerge
   for (unsigned Op : {G_MERGE_VALUES, G_UNMERGE_VALUES}) {
-    unsigned BigTyIdx = Op == G_MERGE_VALUES ? 0 : 1;
-    unsigned LitTyIdx = Op == G_MERGE_VALUES ? 1 : 0;
     auto &MergeUnmergeActions = getActionDefinitionsBuilder(Op);
     if (XLen == 32 && ST.hasStdExtD()) {
-      LLT IdxZeroTy = G_MERGE_VALUES ? s64 : s32;
-      LLT IdxOneTy = G_MERGE_VALUES ? s32 : s64;
-      MergeUnmergeActions.legalFor({IdxZeroTy, IdxOneTy});
+      // Only need to legalize on IdxZeroTy since there is no way to construct
+      // a valid MERGE/UNMERGE where the types don't combine/decompose as
+      // expected.
+      LLT IdxZeroTy = Op == G_MERGE_VALUES ? s64 : s32;
+      MergeUnmergeActions.legalFor({IdxZeroTy});
     }
+    unsigned BigTyIdx = Op == G_MERGE_VALUES ? 0 : 1;
+    unsigned LitTyIdx = Op == G_MERGE_VALUES ? 1 : 0;
     MergeUnmergeActions.widenScalarToNextPow2(LitTyIdx, XLen)
         .widenScalarToNextPow2(BigTyIdx, XLen)
         .clampScalar(LitTyIdx, sXLen, sXLen)

[topperc]

Don't you need to list 2 types here?

[michaelmaitland]

Do we need to?

// Only need to legalize on IdxZeroTy since there is no way to construct
// a valid MERGE/UNMERGE where the types don't combine/decompose as
// expected.

[topperc]

I'm surprised there isn't a check that the correct number of type indices is provided to .legalFor.

[michaelmaitland]

I think there can only be two type indices for these opcodes. A small type or a large type. You cannot construct a MERGE/UNMERGE with >2 types.

[topperc]

I'm surprised that we don't have to pass a 2 element initializer_list to legalFor for an opcode that is defined with 2 type indices. It doesn't make sense to me that you can just omit a type for type index 1.

[michaelmaitland]

I have updated the code to do this, but I suppose there could be a case where you want to mark a type index as legal, regardless of what the other type index is.

[topperc]

I have updated the code to do this, but I suppose there could be a case where you want to mark a type index as legal, regardless of what the other type index is.

You couldn't make type index 1 legal and ignore type index 0 though. So it would be inconsistent if that was the intent.

[topperc]

Can we write

MergeUnmergeActions.legalFor(all(typeIs(BigTyIdx, s64), typeIs(LitTyIdx, s32)))`

[topperc]

LGTM

[RKSimon]

LGTM - thanks @michaelmaitland

[topperc]

This needs to be rebased now