[AIE2P] Implemented VLDB.UNPACK combine #299

katerynamuts · 2025-01-22T09:21:04Z

No description provided.

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp

mludevid

Everything else seems good. The test files need to be checked again once they have been updated.

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp

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

mludevid · 2025-01-22T19:23:49Z

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

+  // Erasing the load instruction breaks later on in the selection code. That is
+  // because we keep an iterator on erased instructions. This breaks while
+  // trying to eliminate a trivially dead instruction which requires access to
+  // its memory operands which have been erased, thus leading to a seg fault. To
+  // remedy this, we keep the load to be removed by the trivial dead code
+  // elimination and we make sure to assign a new virtual register definition to
+  // its live operands to respect SSA.


Nit: I think at this point, we no longer need this comment, we use makeDeadMI in multiple instruction selection methods.

I would rather keep it because if someone reads exactly this function then the comment helps to understand why we do it.

I think explaining it once at the definition of makeDeadMI would suffice. It's just one vscode click away. I also think that the comment is overly woolly. 'We can not erase this since it would invalidate active iterators' says it all.

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

mludevid · 2025-01-22T19:53:23Z

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

Similarly to #292, the offsets in the tests need to be updated, because the immediate ranges have changed from AIE2 to AIE2P. Please also fix them in the post-increment test file.

mludevid · 2025-01-28T10:10:48Z

llvm/lib/Target/AIE/AIEBaseInstructionSelector.cpp

Please update the year in the file header.

mludevid · 2025-01-28T10:11:16Z

llvm/lib/Target/AIE/AIEBaseInstructionSelector.h

Please update the year in the file header.

mludevid · 2025-01-28T10:17:33Z

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp

Could you also use AIEBaseInstructionSelector::setCtrlRegister in selectSetControlRegister?

mludevid · 2025-01-28T10:24:41Z

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

As in AIE2InstructionSelector.cpp: Could you also use AIEBaseInstructionSelector::setCtrlRegister in selectSetControlRegister?

mludevid · 2025-01-28T10:28:38Z

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

@@ -1268,7 +1273,7 @@ bool AIE2PInstructionSelector::selectG_AIE_LOAD_UPS(MachineInstr &UPSI,

  // First use is the G_INTRINSIC_W_SIDE_EFFECTS ID
  Register LoadResult = (std::next(UPSI.uses().begin()))->getReg();
-  MachineInstr *LoadOp = MRI.getUniqueVRegDef(LoadResult);
+  MachineInstr *LoadOp = getDefIgnoringCopiesAndBitcasts(LoadResult, MRI);


Can we add a test for this change?

mludevid · 2025-01-28T10:29:08Z

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

@@ -2672,7 +2677,7 @@ bool AIE2PInstructionSelector::selectG_AIE_STORE_CONV(
  // differ.

  Register ConvResult = (StoreI.uses().begin())->getReg();
-  MachineInstr *ConvOp = MRI.getUniqueVRegDef(ConvResult);
+  MachineInstr *ConvOp = getDefIgnoringCopiesAndBitcasts(ConvResult, MRI);


Same thing as above: Could you add a test for this change?

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

mludevid · 2025-01-28T10:42:51Z

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

+    %8:modregbank(s20) = G_CONSTANT i20 0
+    %9:modregbank(s20) = G_CONSTANT i20 -256
+    %10:modregbank(s20) = G_CONSTANT i20 224
+    %11:modregbank(s20) = G_CONSTANT i20 256


Is there a reason you skipped the offset test with half a step size?

No, I think I just missed it, I added it to the test.

martien-de-jong · 2025-01-28T10:44:36Z

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

+            CombOpInstID == Intrinsic::aie2p_unpack_I1024_I8_I4))) &&
+         "Unexpected VLDB.UNPACK size");
+
+  unsigned ISelOpcode;


I would declare a local version where and when it is needed. Otherwise, please initialize it to 'the illegal opcode'

I cannot create local versions in this case, I would need to give all of them different names, so I initialized it with UINT_MAX.

martien-de-jong · 2025-01-28T10:52:58Z

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

+                         ? AIE2P::VLDB_UNPACK_dmw_ldb_unpack_idx_imm_unpackSign1
+                         : AIE2P::VLDB_UNPACK_dmw_ldb_unpack_idx_unpackSign1;
+        return LoadStoreOpcodes{ISelOpcode, FitsImmediateRange, {}};
+      }


I know the default case doesn't occur, but could we at least put a break between the cases of the outer switch? (if only to help the poor C++ compiler.)

martien-de-jong · 2025-01-28T13:32:11Z

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

+    return false;
+
+  const std::optional<APInt> NoImmediate = {};
+  bool IsSigned = false;


nit: const bool IsSigned = false;

martien-de-jong · 2025-01-28T13:42:38Z

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

+bool AIE2PInstructionSelector::canCombineUNPACKLoad(MachineInstr &MemOp,
+                                                    MachineInstr &CombOp,
+                                                    MachineRegisterInfo &MRI) {
+  Register LoadResult = (std::next(CombOp.uses().begin()))->getReg();


I don't understand this. AFAICS, MemOp is the Load. Why isn't LoadResult the register of its first def?

martien-de-jong · 2025-01-28T13:56:26Z

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

+  const bool AlwaysFitsImmediateRange = true;
+  const bool NoImmediate = false;
+  bool FitsImmediateRange = false;
+  if (IsSigned) {


AFAICS, the else branch differs only in selecting a different opcode. I would arrange for that with a small function unsigned selectSignedness(int Opcode, IsSigned)

it is a good idea and I think it makes sense to do it for all combines but for now I would leave it as it is and implement these functions for all combines in a follow-up PR.

katerynamuts requested a review from mludevid January 22, 2025 09:21

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

andcarminati reviewed Jan 22, 2025

View reviewed changes

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

katerynamuts force-pushed the kmuts.vldb-unpack branch from 837eb63 to cda5d12 Compare January 22, 2025 12:30

mludevid reviewed Jan 22, 2025

View reviewed changes

katerynamuts force-pushed the kmuts.vldb-unpack branch 2 times, most recently from d7580ff to 0e71576 Compare January 24, 2025 08:15

mludevid reviewed Jan 28, 2025

View reviewed changes

martien-de-jong reviewed Jan 28, 2025

View reviewed changes

katerynamuts force-pushed the kmuts.vldb-unpack branch from 0e71576 to 02fcb28 Compare January 29, 2025 11:19

katerynamuts requested review from mludevid, martien-de-jong and andcarminati January 29, 2025 11:19

[AIE2P] Implemented VLDB.UNPACK combine

06b250c

katerynamuts force-pushed the kmuts.vldb-unpack branch from 02fcb28 to 06b250c Compare January 30, 2025 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2P] Implemented VLDB.UNPACK combine #299

[AIE2P] Implemented VLDB.UNPACK combine #299

katerynamuts commented Jan 22, 2025

mludevid left a comment

mludevid Jan 22, 2025

katerynamuts Jan 23, 2025

martien-de-jong Jan 28, 2025 •

edited

Loading

mludevid Jan 22, 2025

mludevid Jan 28, 2025

mludevid Jan 28, 2025

mludevid Jan 28, 2025

mludevid Jan 28, 2025

mludevid Jan 28, 2025

mludevid Jan 28, 2025

mludevid Jan 28, 2025

katerynamuts Jan 29, 2025

martien-de-jong Jan 28, 2025

katerynamuts Jan 29, 2025

martien-de-jong Jan 28, 2025

martien-de-jong Jan 28, 2025

martien-de-jong Jan 28, 2025

martien-de-jong Jan 28, 2025

katerynamuts Jan 29, 2025

[AIE2P] Implemented VLDB.UNPACK combine #299

Are you sure you want to change the base?

[AIE2P] Implemented VLDB.UNPACK combine #299

Conversation

katerynamuts commented Jan 22, 2025

mludevid left a comment

Choose a reason for hiding this 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

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

martien-de-jong Jan 28, 2025 •

edited

Loading