End to end support for bfp16 mac/mul intrinsics #279

niwinanto · 2025-01-17T13:17:35Z

No description provided.

andcarminati · 2025-01-17T15:25:38Z

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

+  case Intrinsic::aie2p_BFP576_BFP1152_ACC2048_mul_conf:
+  case Intrinsic::aie2p_BFP576_BFP1152_ACC2048_negmul_conf:
+    MI = MIB.buildInstr(TII.getOpCode(I), {DstReg},
+                        {Src1Reg, Src2Reg, /*Mode*/ I.getOperand(8).getReg()});


Clang-tidy can help with those comments, like: /*Mode=*/I.getOperand(8).getReg()}

andcarminati · 2025-01-17T15:29:30Z

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

+          .addImm(AIE2P::sub_bfp576_lo)
+          .addReg(Src2RegSub1)
+          .addImm(AIE2P::sub_bfp576_hi);
+  constrainOperandRegClass(*MF, TRI, MRI, TII, RBI, *RegSeq1,


nit: can we have a lambda where we pass something like constrainRegSeq(RegSeq1, AIE2P::VEC512RegClass, 1); ?

andcarminati · 2025-01-17T15:33:08Z

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

@@ -18,6 +18,8 @@
 #include "AIE2PTargetMachine.h"
 #include "AIEBaseInstructionSelector.h"
 #include "MCTargetDesc/aie2p/AIE2PMCTargetDesc.h"
+#include "llvm/ADT/ArrayRef.h"


Those includes are not necessary here.

khallouh

Could you add register bank select tests for these intrinsics? No need to test every single type, only each intrinsic would be enough, as all vector types follow the same code path.

niwinanto · 2025-01-22T05:03:18Z

Could you add register bank select tests for these intrinsics? No need to test every single type, only each intrinsic would be enough, as all vector types follow the same code path.

Done!

khallouh · 2025-01-22T12:34:42Z

clang/lib/Headers/aie2p_vmult.h

@@ -38827,6 +38827,376 @@ addmsc_elem_64_conf(v64bfloat16 a, int sgn_x, v64bfloat16 b, int sgn_y,
                                                            conf);
 };


Nit: 2025 for this file?

khallouh · 2025-01-22T12:35:40Z

llvm/lib/Target/AIE/aie2p/AIE2PInstrPatterns.td

@@ -301,6 +301,45 @@ def : PatInaccessibleMem<(int_aie2p_I512_I512_ACC512_bf_mac_conf VEC512:$s1, VEC
 def : PatInaccessibleMem<(int_aie2p_I512_I512_ACC512_bf_mul_conf VEC512:$s1, VEC512:$s2, eR:$acc),
           (EXTRACT_SUBREG (VMUL_f_vmul_bf_vmul_bf_core_X_X VEC512:$s1, VEC512:$s2, eR:$acc), sub_512_acc_lo)>;


Nit: 2024-2025 in the header

khallouh · 2025-01-22T12:37:37Z

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

+  ConstrainRegSeq(*RegSeq2, AIE2P::EXPVEC64RegClass, 3);
+  ConstrainRegSeq(*RegSeq3, AIE2P::VEC512RegClass, 1);
+  ConstrainRegSeq(*RegSeq3, AIE2P::EXPVEC64RegClass, 3);
+  ConstrainRegSeq(*RegSeq4, AIE2P::mEXaRegClass, 1);


Nit: VEC576 instead of mEXa. You could add this if not already there.

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

khallouh · 2025-01-22T22:31:06Z

llvm/include/llvm/IR/IntrinsicsAIE2P.td

+// BFP16 MAC MUL
+class AIE2PMUL_BFP16V64BFP16
+      : Intrinsic<[llvm_v64i32_ty], [llvm_v64i8_ty, llvm_v8i8_ty, llvm_v64i8_ty, llvm_v8i8_ty, llvm_i32_ty],
+					 [IntrNoMem]>;


These intrinsics read control registers. To model this correctly, we set the following attributes [IntrReadMem, IntrInaccessibleMemOnly] Could you update?

khallouh · 2025-01-22T23:00:53Z

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

@@ -2973,6 +2984,88 @@ AIE2PInstructionSelector::getCombinedOpcodeSRSUPS(
  return {};
 }

+bool AIE2PInstructionSelector ::selectVMAC_BFP(MachineInstr &I,
+                                               MachineRegisterInfo &MRI) {


We can have patterns for all these intrinsics. I tried this one and it works after defining the 2 higher composed sub-register indices. See below:

def : PatInaccessibleMem<(int_aie2p_BFP576_BFP1152_ACC2048_mul_conf VEC512:$s1, EXPVEC64:$s2, VEC512:$s3, VEC512:$s4, EXPVEC64:$s5, EXPVEC64:$s6, eR:$acc), (VMUL_f_vmul_bfp_vmul_bfp_core_EX_EY (REG_SEQUENCE mEXv, VEC512:$s1, sub_bfp16_x, EXPVEC64:$s2, sub_bfp16_e), (REG_SEQUENCE eEY, VEC512:$s3, sub_bfp16_x, EXPVEC64:$s5, sub_bfp16_e, VEC512:$s4, sub_bfp576_hi_bfp16_x, EXPVEC64:$s6, sub_bfp576_hi_bfp16_e), eR:$acc)>;

In AIE2PRegisterInfo.td:

def sub_bfp576_hi_bfp16_e : ComposedSubRegIndex<sub_bfp576_hi, sub_bfp16_e>; def sub_bfp576_hi_bfp16_x : ComposedSubRegIndex<sub_bfp576_hi, sub_bfp16_x>;

@khallouh This is really nice. I could not make it working with REG_SEQUENCE. Thanks for spending time on it.

khallouh

Could you add the intrinsics to the isAccIntrinsic in RegBankSelect? And to test this store the result of the MAC operations instead of copying it into an accumulator register.
The rest looks good, thanks for addressing the comments.

niwinanto · 2025-01-23T10:55:10Z

Could you add the intrinsics to the isAccIntrinsic in RegBankSelect? And to test this store the result of the MAC operations instead of copying it into an accumulator register. The rest looks good, thanks for addressing the comments.

Done. For the register bank tests instead of copying to accumulator register, returning the virtual register. This will do the trick.

khallouh

LGTM!

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

andcarminati reviewed Jan 17, 2025

View reviewed changes

niwinanto force-pushed the niwin.bfp16.mac_mul2 branch 2 times, most recently from 879d252 to 94481ed Compare January 20, 2025 11:20

khallouh reviewed Jan 20, 2025

View reviewed changes

niwinanto force-pushed the niwin.bfp16.mac_mul2 branch from 94481ed to 97bbf52 Compare January 22, 2025 05:00

niwinanto requested review from F-Stuckmann and katerynamuts as code owners January 22, 2025 05:00

khallouh reviewed Jan 22, 2025

View reviewed changes

llvm/lib/Target/AIE/aie2p/AIE2PRegisterBankInfo.cpp Show resolved Hide resolved

khallouh reviewed Jan 22, 2025

View reviewed changes

niwinanto force-pushed the niwin.bfp16.mac_mul2 branch 2 times, most recently from 08814d5 to 050b95c Compare January 23, 2025 07:55

[AIE2P] Support bfp16 mac/mul intrinsics - frontend

b361d6b

niwinanto force-pushed the niwin.bfp16.mac_mul2 branch from 050b95c to 535eba3 Compare January 23, 2025 09:39

khallouh reviewed Jan 23, 2025

View reviewed changes

[AIE2P] Support bfp16 mac/mul intrinsics - backend

e66e24e

niwinanto force-pushed the niwin.bfp16.mac_mul2 branch from 535eba3 to e66e24e Compare January 23, 2025 10:51

khallouh approved these changes Jan 23, 2025

View reviewed changes

niwinanto merged commit 81f4c1f into aie-public Jan 23, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

End to end support for bfp16 mac/mul intrinsics #279

End to end support for bfp16 mac/mul intrinsics #279

niwinanto commented Jan 17, 2025

andcarminati Jan 17, 2025

andcarminati Jan 17, 2025

niwinanto Jan 22, 2025

andcarminati Jan 17, 2025

khallouh left a comment

niwinanto commented Jan 22, 2025

khallouh Jan 22, 2025

khallouh Jan 22, 2025

khallouh Jan 22, 2025

khallouh Jan 22, 2025

khallouh Jan 22, 2025

niwinanto Jan 23, 2025

khallouh left a comment •

edited

Loading

niwinanto commented Jan 23, 2025

khallouh left a comment

		@@ -38827,6 +38827,376 @@ addmsc_elem_64_conf(v64bfloat16 a, int sgn_x, v64bfloat16 b, int sgn_y,
		conf);
		};

		@@ -301,6 +301,45 @@ def : PatInaccessibleMem<(int_aie2p_I512_I512_ACC512_bf_mac_conf VEC512:$s1, VEC
		def : PatInaccessibleMem<(int_aie2p_I512_I512_ACC512_bf_mul_conf VEC512:$s1, VEC512:$s2, eR:$acc),
		(EXTRACT_SUBREG (VMUL_f_vmul_bf_vmul_bf_core_X_X VEC512:$s1, VEC512:$s2, eR:$acc), sub_512_acc_lo)>;

End to end support for bfp16 mac/mul intrinsics #279

End to end support for bfp16 mac/mul intrinsics #279

Conversation

niwinanto commented Jan 17, 2025

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

khallouh left a comment

Choose a reason for hiding this comment

niwinanto commented Jan 22, 2025

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

khallouh left a comment • edited Loading

Choose a reason for hiding this comment

niwinanto commented Jan 23, 2025

khallouh left a comment

Choose a reason for hiding this comment

khallouh left a comment •

edited

Loading