[AIE2P] Instruction select G_SELECT. #291

SagarMaheshwari99 · 2025-01-20T18:27:06Z

No description provided.

konstantinschwarz

Few suggestions to refactor some code, overall this look ok

konstantinschwarz · 2025-01-20T19:08:29Z

llvm/lib/Target/AIE/AIE2InstrPatterns.td

-  def : Pat<(vec512Ty (select (i32 eRS8:$rs1), VEC512:$rs2, VEC512:$rs3)),
-            (vec512Ty (VSEL_32 VEC512:$rs2, VEC512:$rs3, (ADD_add_r_ri eR:$rs1, (i32 -1))))>;
-}
-foreach vec1024Ty = [v128i8, v64i16, v32i32] in {


Can we adapt the legalization rules for G_SELECT and clamp to 512-bit vector types?
Then we only need to support the 512-bit patterns

Its a nice idea.

May be I have one more suggestion, since we only allow 512-bit vector types. Can we get rid of explicitly converting accumulator to vector by bitcasts. And constrain G_SELECT to vector register banks in the regbank selection.

But we are just delaying this action from legalizer to regbankselect?
I think nothing should pass legalizer, if we don't intend it to be legal.

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vsel.mir

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/legalize-vsel.mir

niwinanto

Change looks good. Few nits and a suggestion.

llvm/lib/Target/AIE/AIE2LegalizerInfo.cpp

niwinanto · 2025-01-24T13:48:41Z

llvm/lib/Target/AIE/AIE2InstrPatterns.td

-  def : Pat<(vec512Ty (select (i32 eRS8:$rs1), VEC512:$rs2, VEC512:$rs3)),
-            (vec512Ty (VSEL_32 VEC512:$rs2, VEC512:$rs3, (ADD_add_r_ri eR:$rs1, (i32 -1))))>;
-}
-foreach vec1024Ty = [v128i8, v64i16, v32i32] in {


Its a nice idea.

May be I have one more suggestion, since we only allow 512-bit vector types. Can we get rid of explicitly converting accumulator to vector by bitcasts. And constrain G_SELECT to vector register banks in the regbank selection.

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

llvm/lib/Target/AIE/AIE2LegalizerInfo.cpp

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

martien-de-jong · 2025-01-28T15:40:12Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+
+  const Register NewDstReg = MRI.createGenericVirtualRegister(NewVecTy);
+  MIRBuilder.buildInstr(MI.getOpcode(), {NewDstReg},
+                        {SrcReg0, NewSrcReg1, NewSrcReg2}, MI.getFlags());


Check: SrcReg0 is false or true, which correctly defines a vector condition for element 0, the only one we're interested in. (Perhaps give an outline of this strategy in a comment at the start of the function)

G_SELECT only gets a 0/1 value as condition, and selects either SrcReg1 if condition is true, i.e. 1, or SrcReg2 if condition is false, i.e. 0.

AIE's instruction works slightly differently:

Each bit in sel selects either from xsrc0 (value zero) or xsrc1 (value one).

Hence we need to translate the 0/1 bit to a bitmask in instruction selection.
We do this using cond - 1. Which will result in a zero mask if cond is true, or an all 1 mask (-1) if cond is false.

Sorry for my confusion. We just legalize to the vector size by padding, we don't map to VSEL here.
But note that you could leave the condition as it is for vector sizes <= 32, because we have 8, 16 and 32 bit vsel.

konstantinschwarz · 2025-01-29T18:41:29Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+
+  const Register NewDstReg = MRI.createGenericVirtualRegister(NewVecTy);
+  MIRBuilder.buildInstr(MI.getOpcode(), {NewDstReg},
+                        {SrcReg0, NewSrcReg1, NewSrcReg2}, MI.getFlags());


G_SELECT only gets a 0/1 value as condition, and selects either SrcReg1 if condition is true, i.e. 1, or SrcReg2 if condition is false, i.e. 0.

AIE's instruction works slightly differently:

Each bit in sel selects either from xsrc0 (value zero) or xsrc1 (value one).

Hence we need to translate the 0/1 bit to a bitmask in instruction selection.
We do this using cond - 1. Which will result in a zero mask if cond is true, or an all 1 mask (-1) if cond is false.

konstantinschwarz · 2025-01-29T18:42:33Z

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/regbankselect-special-cases.mir

-# RUN: llc -mtriple aie2p -run-pass=regbankselect -regbankselect-greedy %s -verify-machineinstrs -o - | FileCheck --check-prefix=FAST %s
+# (c) Copyright 2024-2025 Advanced Micro Devices, Inc. or its affiliates
+
+# RUN: llc -mtriple aie2p -run-pass=legalizer,regbankselect -regbankselect-fast %s -verify-machineinstrs -o - | FileCheck --check-prefix=GREEDY %s


I would prefer to not run the legalizer in a regbankselect test.
We can simply remove the illegal cases from this test, as long as we cover them in the legalizer test

ok removed.

martien-de-jong · 2025-01-30T12:48:12Z

llvm/lib/Target/AIE/AIE2LegalizerInfo.cpp

+    return QueryTy.isVector() && QueryTy.getSizeInBits() < Size;
+  };
+}
+
 LegalityPredicate
 negatePredicate(const std::function<bool(const LegalityQuery &)> &Func) {
  return [=](const LegalityQuery &Query) { return !Func(Query); };


Oh, I wouldn't dare writing this. That's a reference to a function handle abstraction captured by value into a lambda somehow bound to a LegailityPredicate return value.

martien-de-jong · 2025-01-30T12:57:59Z

llvm/lib/Target/AIE/AIE2LegalizerInfo.cpp

@@ -236,10 +243,24 @@ AIE2LegalizerInfo::AIE2LegalizerInfo(const AIE2Subtarget &ST) : AIEHelper(ST) {

  getActionDefinitionsBuilder(G_SELECT)
      .legalFor({{S32, S32}, {P0, S32}})
+      .clampScalar(1, S32, S32)
+      // AIE ISA supports only 512-bit vector select


That is AIE2/AIE2P I guess.

martien-de-jong · 2025-01-30T13:04:56Z

llvm/lib/Target/AIE/AIE2LegalizerInfo.cpp

+      .bitcastIf(
+          [=](const LegalityQuery &Query) {
+            const LLT &ResTy = Query.Types[0];
+            return ResTy.isScalar() && ResTy.getSizeInBits() >= 256;


When do we see scalars >= 256?

For AIE2, we probably don't.

martien-de-jong · 2025-01-30T13:36:13Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

  const LLT DstTy = MRI.getType(DstReg);
+
+  if (DstTy.isVector() && DstTy.getSizeInBits() < 512)
+    return legalizeG_SELECTLessThan512Bit(Helper, MI);


I think we should parameterize this with the size limit and not have 512 in the name.

I kept it coz we only had 512bit "max" bit size possible, but I changed it now.

andcarminati · 2025-01-30T16:09:00Z

llvm/lib/Target/AIE/AIELegalizerHelper.cpp

+
+  if (DstTy.isVector() && DstTy.getSizeInBits() < 512)
+    return legalizeG_SELECTWithSizeLimit(Helper, MI, 512);
+
  assert(DstTy.isVector() && DstTy.getSizeInBits() == 2048 &&


I have a feeling that you can drop all this 2048 handling if:

Wait for this change to be integrated: https://github.com/Xilinx/llvm-aie/blob/c01a09a08e6c1295a19373f7871da16a394c9e03/llvm/lib/Target/AIE/aie2p/AIE2PLegalizerInfo.cpp#L522C1-L558C13 (it will apply fewer elements if for unmerge with more than 2 dest regs.)

Remove your rule .customFor({{AccV64S32, S32}}).

As result, .clampMaxNumElements(0, S32, 16) that is already there will do the job for us.

This will change your test in the following way:

; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(<64 x s32>) = COPY $dm1 ; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(<64 x s32>) = COPY $dm2 ; CHECK-NEXT: [[UV:%[0-9]+]]:_(<32 x s32>), [[UV1:%[0-9]+]]:_(<32 x s32>) = G_UNMERGE_VALUES [[COPY1]](<64 x s32>) - ; CHECK-NEXT: [[UV2:%[0-9]+]]:_(<32 x s32>), [[UV3:%[0-9]+]]:_(<32 x s32>) = G_UNMERGE_VALUES [[COPY2]](<64 x s32>) - ; CHECK-NEXT: [[UV4:%[0-9]+]]:_(<16 x s32>), [[UV5:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV]](<32 x s32>) - ; CHECK-NEXT: [[UV6:%[0-9]+]]:_(<16 x s32>), [[UV7:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV2]](<32 x s32>) - ; CHECK-NEXT: [[SELECT:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV4]], [[UV6]] - ; CHECK-NEXT: [[SELECT1:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV5]], [[UV7]] + ; CHECK-NEXT: [[UV2:%[0-9]+]]:_(<16 x s32>), [[UV3:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV]](<32 x s32>) + ; CHECK-NEXT: [[UV4:%[0-9]+]]:_(<16 x s32>), [[UV5:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV1]](<32 x s32>) + ; CHECK-NEXT: [[UV6:%[0-9]+]]:_(<32 x s32>), [[UV7:%[0-9]+]]:_(<32 x s32>) = G_UNMERGE_VALUES [[COPY2]](<64 x s32>) + ; CHECK-NEXT: [[UV8:%[0-9]+]]:_(<16 x s32>), [[UV9:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV6]](<32 x s32>) + ; CHECK-NEXT: [[UV10:%[0-9]+]]:_(<16 x s32>), [[UV11:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV7]](<32 x s32>) + ; CHECK-NEXT: [[SELECT:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV2]], [[UV8]] + ; CHECK-NEXT: [[SELECT1:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV3]], [[UV9]] + ; CHECK-NEXT: [[SELECT2:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV4]], [[UV10]] + ; CHECK-NEXT: [[SELECT3:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV5]], [[UV11]] ; CHECK-NEXT: [[CONCAT_VECTORS:%[0-9]+]]:_(<32 x s32>) = G_CONCAT_VECTORS [[SELECT]](<16 x s32>), [[SELECT1]](<16 x s32>) - ; CHECK-NEXT: [[UV8:%[0-9]+]]:_(<16 x s32>), [[UV9:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV1]](<32 x s32>) - ; CHECK-NEXT: [[UV10:%[0-9]+]]:_(<16 x s32>), [[UV11:%[0-9]+]]:_(<16 x s32>) = G_UNMERGE_VALUES [[UV3]](<32 x s32>) - ; CHECK-NEXT: [[SELECT2:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV8]], [[UV10]] - ; CHECK-NEXT: [[SELECT3:%[0-9]+]]:_(<16 x s32>) = G_SELECT [[ASSERT_ZEXT]](s32), [[UV9]], [[UV11]] ; CHECK-NEXT: [[CONCAT_VECTORS1:%[0-9]+]]:_(<32 x s32>) = G_CONCAT_VECTORS [[SELECT2]](<16 x s32>), [[SELECT3]](<16 x s32>) ; CHECK-NEXT: [[CONCAT_VECTORS2:%[0-9]+]]:_(<64 x s32>) = G_CONCAT_VECTORS [[CONCAT_VECTORS]](<32 x s32>), [[CONCAT_VECTORS1]](<32 x s32>) ; CHECK-NEXT: $dm0 = COPY [[CONCAT_VECTORS2]](<64 x s32>)

What do you think?

I have made the changes.

konstantinschwarz

Looks good!

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

SagarMaheshwari99 force-pushed the sagarm.gselect branch from a922878 to a917d57 Compare January 20, 2025 18:27

konstantinschwarz reviewed Jan 20, 2025

View reviewed changes

SagarMaheshwari99 force-pushed the sagarm.gselect branch from a917d57 to f8c5009 Compare January 23, 2025 15:25

SagarMaheshwari99 requested review from F-Stuckmann, katerynamuts and niwinanto as code owners January 23, 2025 15:25

katerynamuts reviewed Jan 23, 2025

View reviewed changes

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/legalize-vsel.mir Outdated Show resolved Hide resolved

SagarMaheshwari99 force-pushed the sagarm.gselect branch 2 times, most recently from aaa8426 to 63d335f Compare January 24, 2025 12:38

niwinanto reviewed Jan 24, 2025

View reviewed changes

martien-de-jong reviewed Jan 28, 2025

View reviewed changes

llvm/lib/Target/AIE/AIELegalizerHelper.cpp Outdated Show resolved Hide resolved

martien-de-jong reviewed Jan 28, 2025

View reviewed changes

llvm/lib/Target/AIE/AIELegalizerHelper.cpp Outdated Show resolved Hide resolved

martien-de-jong reviewed Jan 28, 2025

View reviewed changes

SagarMaheshwari99 force-pushed the sagarm.gselect branch 2 times, most recently from f1b8457 to 68c7849 Compare January 29, 2025 16:23

konstantinschwarz reviewed Jan 29, 2025

View reviewed changes

SagarMaheshwari99 force-pushed the sagarm.gselect branch from 68c7849 to 65a1092 Compare January 30, 2025 12:19

martien-de-jong reviewed Jan 30, 2025

View reviewed changes

SagarMaheshwari99 force-pushed the sagarm.gselect branch from 65a1092 to 9a2c768 Compare January 30, 2025 15:24

andcarminati reviewed Jan 30, 2025

View reviewed changes

SagarMaheshwari99 force-pushed the sagarm.gselect branch from 9a2c768 to 3307a20 Compare January 31, 2025 14:18

[AIE2][AIE2P] Legalize G_SELECT for 512 bits only.

b1b39ab

SagarMaheshwari99 force-pushed the sagarm.gselect branch from 3307a20 to e5ca8e3 Compare January 31, 2025 17:31

[AIE2][AIE2P] Instruction select G_SELECT.

0f293f5

konstantinschwarz previously approved these changes Jan 31, 2025

View reviewed changes

SagarMaheshwari99 dismissed konstantinschwarz’s stale review via 0f293f5 January 31, 2025 17:40

SagarMaheshwari99 force-pushed the sagarm.gselect branch from e5ca8e3 to 0f293f5 Compare January 31, 2025 17:40

konstantinschwarz approved these changes Jan 31, 2025

View reviewed changes

SagarMaheshwari99 enabled auto-merge (rebase) January 31, 2025 17:43

SagarMaheshwari99 disabled auto-merge January 31, 2025 17:43

SagarMaheshwari99 enabled auto-merge (rebase) January 31, 2025 17:43

SagarMaheshwari99 merged commit dc6ec0b into aie-public Jan 31, 2025
8 checks passed

konstantinschwarz deleted the sagarm.gselect branch January 31, 2025 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2P] Instruction select G_SELECT. #291

[AIE2P] Instruction select G_SELECT. #291

SagarMaheshwari99 commented Jan 20, 2025

konstantinschwarz left a comment

konstantinschwarz Jan 20, 2025

SagarMaheshwari99 Jan 23, 2025

niwinanto Jan 24, 2025

SagarMaheshwari99 Jan 29, 2025

niwinanto left a comment

niwinanto Jan 24, 2025

martien-de-jong Jan 28, 2025 •

edited

Loading

SagarMaheshwari99 Jan 29, 2025

konstantinschwarz Jan 29, 2025

martien-de-jong Jan 30, 2025

konstantinschwarz Jan 29, 2025

konstantinschwarz Jan 29, 2025

SagarMaheshwari99 Jan 30, 2025

martien-de-jong Jan 30, 2025

martien-de-jong Jan 30, 2025

martien-de-jong Jan 30, 2025

SagarMaheshwari99 Jan 30, 2025

martien-de-jong Jan 30, 2025

SagarMaheshwari99 Jan 30, 2025

andcarminati Jan 30, 2025 •

edited

Loading

SagarMaheshwari99 Jan 31, 2025

konstantinschwarz left a comment

[AIE2P] Instruction select G_SELECT. #291

[AIE2P] Instruction select G_SELECT. #291

Conversation

SagarMaheshwari99 commented Jan 20, 2025

konstantinschwarz left a 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

niwinanto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martien-de-jong Jan 28, 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

andcarminati Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstantinschwarz left a comment

Choose a reason for hiding this comment

martien-de-jong Jan 28, 2025 •

edited

Loading

andcarminati Jan 30, 2025 •

edited

Loading