[AIE2P] Legalize G_UNMERGE_VALUES 256-bit vector to 2 128-bit vectors #282

niwinanto · 2025-01-17T17:09:50Z

This PR introduces G_AIE_VSHIFT_RIGHT gMIR opcode which is selected to vshift instruction. And this is utilized to legalize G_UNMERGE_VALUES 256-bit vector to 2 128-bit vector.

konstantinschwarz · 2025-01-17T21:01:34Z

llvm/lib/Target/AIE/aie2p/AIE2PLegalizerInfo.cpp

@@ -551,8 +551,10 @@ AIE2PLegalizerInfo::AIE2PLegalizerInfo(const AIE2PSubtarget &ST)
        const LLT &DstTy = Query.Types[0];
        const LLT &SrcTy = Query.Types[1];

-        return SrcTy.isVector() && DstTy.isScalar() &&
-               DstTy == SrcTy.getElementType();
+        return (DstTy.isVector() && SrcTy.getSizeInBits() == 256 &&


Could you add a comment to explain which cases this handles? (for both the existing and new case)

andcarminati · 2025-01-20T13:13:01Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

@@ -231,6 +231,57 @@ bool AIELegalizerHelper::legalizeG_BUILD_VECTOR(LegalizerHelper &Helper,
  return true;
 }



Can we have a description of the input->output of this custom legalization rule?

andcarminati · 2025-01-20T13:23:22Z

llvm/lib/Target/AIE/AIEInstrGISel.td

+
+// Concatenate the src1 and src2 vectors, shift right
+// and extract the resulting lower 512-bit vector
+def G_AIE_VSHIFT : AIEGenericInstruction {


I was expecting "right" to be visible in the opcode itself. If I see this opcode there is no way to discover that it does right shift without looking to the comment.

andcarminati · 2025-01-20T13:49:21Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+  const AIEBaseInstrInfo *II = ST.getInstrInfo();
+  const unsigned UnpadOpc = II->getGenericUnpadVectorOpcode();
+  const Register SrcReg = MI.getOperand(MI.getNumOperands() - 1).getReg();
+  const Register Dst1Reg = MI.getOperand(0).getReg();


nit: maybe DstRegLow and DstRegHigh to improve readability.

andcarminati · 2025-01-20T14:00:54Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+      MIRBuilder.buildInstr(TargetOpcode::G_IMPLICIT_DEF, {SrcTy}, {})
+          .getReg(0);
+
+  const LLT Vec512 =


nit: const LLT Vec512 = SrcTy.multiplyElements(2);

andcarminati · 2025-01-20T14:07:31Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+
+  // VSHIFT operates on 512-bit inputs. We need to pad the 256-bit source
+  // operand to 512-bit
+  const Register ImplicitDef256 =


nit: const Register ImplicitDef256 = MIRBuilder.buildUndef(SrcTy).getReg(0);

andcarminati · 2025-01-20T14:07:52Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+      MIRBuilder.buildConcatVectors({Vec512}, {SrcReg, ImplicitDef256});
+
+  // The second input will be ignored. Just create a dummy input
+  auto ImplicitDef512 =


nit: auto ImplicitDef512 = MIRBuilder.buildUndef(Vec512);

andcarminati · 2025-01-21T09:27:49Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+//  to
+//     %1:_(<4 x s32>) = G_AIE_UNPAD_VECTOR %0(<8 x s32>)
+//     %3:_(s32) = G_CONSTANT i32 16
+//     %4:_(<16 x s32>) = G_CONCAT_VECTORS %0(<8 x s32>), %0(<8 x s32>)


I guess that %0 is G_IMPLICIT_DEF, right?

Yes, that was not important. That`s why I left it.

andcarminati · 2025-01-21T09:29:13Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+  const LLT Vec512 = SrcTy.multiplyElements(2);
+
+  // Create the first 512-bit vector input
+  auto MergeValue =


If you don't mind, I think ConcatValue sounds better here.

andcarminati · 2025-01-21T09:31:54Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+  const Register DstRegLow = MI.getOperand(0).getReg();
+  const Register DstRegHigh = MI.getOperand(1).getReg();
+  const LLT SrcTy = MRI.getType(SrcReg);
+


Maybe we can assert that SrcTy is a vector type to avoid misuse.

Added assert for destination register, it ensure src to be vector as well.

andcarminati · 2025-01-21T09:39:42Z

llvm/lib/Target/AIE/AIELegalizerHelper.h

@@ -68,6 +68,7 @@ class AIELegalizerHelper {
                       Register SourceReg) const;
  bool unpack32BitVector(LegalizerHelper &Helper, MachineInstr &MI,
                         Register SourceReg) const;
+  bool legalize_unmerge_128bit(LegalizerHelper &Helper, MachineInstr &MI) const;


nit: what about the standard naming convention legalizeUnmergeTo128bit.

Makes sense!

Hmm, other function names in the file follow this, legalizeG_SEXT_INREG, legalizeG_VASTART ...

So, legalizeG_UNMERGE... would fit in this case.

andcarminati · 2025-01-21T13:44:32Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+  const Register DstRegHigh = MI.getOperand(1).getReg();
+  const LLT SrcTy = MRI.getType(SrcReg);
+
+  assert(MRI.getType(DstRegLow).isVector() && "Expected vector restor");


nit: restor is register?

andcarminati · 2025-01-21T13:46:13Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

@@ -241,6 +299,11 @@ bool AIELegalizerHelper::legalizeG_UNMERGE_VALUES(LegalizerHelper &Helper,
  const Register LastReg = MI.getOperand(MI.getNumOperands() - 1).getReg();
  const LLT FirstTy = MRI.getType(FirstReg);
  const LLT LastTy = MRI.getType(LastReg);
+
+  if (ST.isAIE2P() && FirstTy.getSizeInBits() == 128 &&


Maybe check for vector types here?

andcarminati

LGTM.

niwinanto requested review from abhinay-anubola, abnikant, andcarminati, gbossu, khallouh, konstantinschwarz, martien-de-jong, SagarMaheshwari99 and stephenneuendorffer as code owners January 17, 2025 17:09

niwinanto force-pushed the niwin.vshift.unmerge branch 2 times, most recently from 7ef0b48 to 8df98bb Compare January 17, 2025 17:15

konstantinschwarz reviewed Jan 17, 2025

View reviewed changes

niwinanto force-pushed the niwin.vshift.unmerge branch from 8df98bb to a2d74e1 Compare January 20, 2025 08:37

andcarminati reviewed Jan 20, 2025

View reviewed changes

niwinanto force-pushed the niwin.vshift.unmerge branch from a2d74e1 to fa09112 Compare January 21, 2025 07:38

[AIE2P] Introduce AIE_VSHIFT gMIR opcode and instruction select

739dea5

niwinanto force-pushed the niwin.vshift.unmerge branch from fa09112 to 9f4905e Compare January 21, 2025 08:26

andcarminati reviewed Jan 21, 2025

View reviewed changes

niwinanto force-pushed the niwin.vshift.unmerge branch 2 times, most recently from 83a1eff to ea7999a Compare January 21, 2025 09:38

andcarminati reviewed Jan 21, 2025

View reviewed changes

niwinanto force-pushed the niwin.vshift.unmerge branch from ea7999a to c86cb0a Compare January 21, 2025 12:29

andcarminati reviewed Jan 21, 2025

View reviewed changes

[AIE2P] Legalize G_UNMERGE_VALUES 256-bit vector to 2 128-bit vectors

d6e46e6

niwinanto force-pushed the niwin.vshift.unmerge branch from c86cb0a to d6e46e6 Compare January 21, 2025 13:52

andcarminati approved these changes Jan 21, 2025

View reviewed changes

niwinanto merged commit 7108f7f into aie-public Jan 21, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2P] Legalize G_UNMERGE_VALUES 256-bit vector to 2 128-bit vectors #282

[AIE2P] Legalize G_UNMERGE_VALUES 256-bit vector to 2 128-bit vectors #282

niwinanto commented Jan 17, 2025 •

edited

Loading

konstantinschwarz Jan 17, 2025

andcarminati Jan 20, 2025

niwinanto Jan 21, 2025

andcarminati Jan 20, 2025

niwinanto Jan 21, 2025

andcarminati Jan 20, 2025

niwinanto Jan 21, 2025

andcarminati Jan 20, 2025

andcarminati Jan 20, 2025

andcarminati Jan 20, 2025

andcarminati Jan 21, 2025

niwinanto Jan 21, 2025

andcarminati Jan 21, 2025

niwinanto Jan 21, 2025

niwinanto Jan 21, 2025

andcarminati Jan 21, 2025

niwinanto Jan 21, 2025

andcarminati Jan 21, 2025

niwinanto Jan 21, 2025

niwinanto Jan 21, 2025

andcarminati Jan 21, 2025

niwinanto Jan 21, 2025

andcarminati Jan 21, 2025

andcarminati Jan 21, 2025

andcarminati left a comment

		@@ -231,6 +231,57 @@ bool AIELegalizerHelper::legalizeG_BUILD_VECTOR(LegalizerHelper &Helper,
		return true;
		}

[AIE2P] Legalize G_UNMERGE_VALUES 256-bit vector to 2 128-bit vectors #282

[AIE2P] Legalize G_UNMERGE_VALUES 256-bit vector to 2 128-bit vectors #282

Conversation

niwinanto commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andcarminati left a comment

Choose a reason for hiding this comment

niwinanto commented Jan 17, 2025 •

edited

Loading