[AIE2P] End-to-End support of fifo store intrinsics #308

khallouh · 2025-01-23T14:06:04Z

No description provided.

llvm/include/llvm/IR/IntrinsicsAIE2P.td

gbossu · 2025-01-23T14:57:47Z

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

+    case Intrinsic::aie2p_fifo_st_push_512_bfp16:
+    case Intrinsic::aie2p_fifo_st_flush:
+    case Intrinsic::aie2p_fifo_st_flush_conv:
+    case Intrinsic::aie2p_fifo_st_flush_1d:


Would that be equivalent to what you call a normal aie2p_fifo_st_flush followed by a ptr increment? Same for 2d/3d?

I would hope so. And the architecture file calls it normal ;-)

I was just wondering because we would then only need the aie2p_fifo_st_flush instrinsic/builtin. And we could get the 1d/2d/3d versions using instruction selection combiners like we do for "normal" load/stores. But then we would probably need new G_AIE_FIFO_xxxx opcodes, so it's probably not worth to do this now.

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

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/regbankselect-fifo-insn.mir

clang/lib/CodeGen/CGBuiltin.cpp

clang/test/CodeGen/aie/aie2p/aie2p-ldst.cpp

gbossu · 2025-01-24T10:54:29Z

clang/test/CodeGen/aie/aie2p/aie2p-ldst.cpp

+void test_fifo_st_flush_2d_byte(v16uint32 *&p, fifo_state_t &s, int off,
+                                int size1, addr_t &count1, int inc1) {
+  return fifo_st_flush_2d_byte(p, s, off, size1, count1, inc1);
+}


Nit: This really is a huge file and it is pretty much impossible to review. It seems that in many cases, only the vector type changes. Do you think it would be possible to templatise all those test_xxxx functions and run the same file multiple times with e.g. -DTYPE=v16uint32, -DTYPE=v16int32, etc.

At least I think that would work for the "flush" tests, because the vector type does not seem to appear in the check lines. For "push", we might have so write a test for each type, but there aren't that many push variants, so I think that's fine.

I tried it but there is still the function label line which depends on the type.

llvm/test/CodeGen/AIE/aie2p/ldst-fifo-stores.ll

clang/test/CodeGen/aie/aie2p/aie2p-ldst.cpp

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

clang/lib/Headers/aie2p_ldst.h

konstantinschwarz · 2025-01-27T19:18:30Z

clang/lib/Headers/aie2p_ldst.h

+FIFO_ST_NORMAL(v32int16)
+FIFO_ST_NORMAL(v32uint16)
+FIFO_ST_NORMAL(v16int32)
+FIFO_ST_NORMAL(v16uint32)


Should #undef these macros after their usage

konstantinschwarz · 2025-01-27T19:19:39Z

clang/lib/Headers/aie2p_ldst.h

@@ -81,4 +81,118 @@ INTRINSIC(v64uint16) unpack(v64uint8 v) { return unpack(v, __SIGN_UNSIGNED); }
 INTRINSIC(v128int8) unpack(v128int4 v) { return unpack(v, __SIGN_SIGNED); }
 INTRINSIC(v128uint8) unpack(v128uint4 v) { return unpack(v, __SIGN_UNSIGNED); }

+#define FIFO_ST_PUSH_NORMAL(T)                                                 \
+  INTRINSIC(void) fifo_st_reset(T *&p, T v, fifo_state_t &s) {                 \


The API also supports volatile qualified pointers. Could you add those variants as well?

Correction: I meant restrict, not volatile

I have added the versions with memory bank annotations and the restrict qualifier. See the last fixup commit.

abhinay-anubola · 2025-01-28T11:44:35Z

It would be good if you could squash the fixup commit with the original commit. This will help keep the commit history clean.

khallouh · 2025-01-28T13:18:48Z

It would be good if you could squash the fixup commit with the original commit. This will help keep the commit history clean.

I will do that later. Just wanted to show what the fixup commit fixed and when.

clang/test/CodeGen/aie/aie2p/aie2p-ldst.cpp

gbossu · 2025-01-29T13:20:24Z

clang/test/CodeGen/aie/aie-bank-annotation.cpp

 // RUN: %clang -O1 --target=aie2 -nostdlibinc -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang -O1 --target=aie2p -nostdlibinc -S -emit-llvm %s -o - | FileCheck %s


gbossu

LGTM. I cannot verify every line, but what I've seen is correct and our regression suite seems to confirm that. Nice work!

khallouh requested review from abhinay-anubola, abnikant, andcarminati, F-Stuckmann, gbossu, katerynamuts, konstantinschwarz, martien-de-jong, niwinanto, SagarMaheshwari99 and stephenneuendorffer as code owners January 23, 2025 14:06

gbossu reviewed Jan 23, 2025

View reviewed changes

llvm/include/llvm/IR/IntrinsicsAIE2P.td Show resolved Hide resolved

gbossu reviewed Jan 23, 2025

View reviewed changes

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-fifo-stores.mir Outdated Show resolved Hide resolved

gbossu reviewed Jan 23, 2025

View reviewed changes

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/regbankselect-fifo-insn.mir Show resolved Hide resolved

khallouh force-pushed the hamza.bfp16.ldst.store branch from 5de2dfb to 25d6fdd Compare January 24, 2025 08:49