[AIE2] Enable up-combining for VLD + UNPACK #241

andcarminati · 2024-11-20T11:09:08Z

If we can't delay VLD, we can try to antecipate UNPACK. This approach can be extended to other selection combiners.

andcarminati · 2024-11-20T11:09:35Z

QoR:

|--------------------------------------------------------------|------------|----------|--------------|
| Core_Compute_Cycle_Count                                     | aie-public | This PR. | Total diff   |
|--------------------------------------------------------------|------------|----------|--------------|
| Softmax_bf16_0                                               |       6350 |     6351 | SAME(+0.02%) |
|--------------------------------------------------------------|------------|----------|--------------|
| ThresholdedRelu_aie2_int8                                    |        865 |      849 | IMPR(-1.85%) |
|--------------------------------------------------------------|------------|----------|--------------|
| Erf_aie2_int8_0                                              |       2554 |     2490 | IMPR(-2.51%) |
|--------------------------------------------------------------|------------|----------|--------------|
| Erf_aie2_int8_0_ptr_interface                                |       2533 |     2469 | IMPR(-2.53%) |
|--------------------------------------------------------------|------------|----------|--------------|
| HardswishAsHardsigmoid_aie2_0                                |       1368 |     1240 | IMPR(-9.36%) |
|--------------------------------------------------------------|------------|----------|--------------|
| Hardswish_aie2_0                                             |       1368 |     1240 | IMPR(-9.36%) |
|--------------------------------------------------------------|------------|----------|--------------|
| Averege diff                                                 |            | -0.07%   | -0.07%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Diff stdev                                                   |            |     0.75 |         0.75 |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #1                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #2                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #3                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #4                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #5                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #6                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #7                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #8                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|
| Quantile #9                                                  |            | +0.00%   | +0.00%       |
|--------------------------------------------------------------|------------|----------|--------------|

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp

llvm/lib/Target/AIE/AIECombinerHelper.cpp

gbossu · 2024-11-20T13:23:45Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

-bool isUseOf(const MachineInstr &MI, const MachineInstr &Use) {
-  for (auto &Defs : Use.defs()) {
+bool isUseOf(const MachineInstr &MI, const MachineInstr &Def) {
+  for (auto &Defs : Def.defs()) {


Does that iterate on implicit defs as well? I'm never sure

No, just explicit ones! To cover all definitions, we need all_defs()

I guess we should be careful about implicit defs as well. The instruction might write to a control register that is used by a later instruction. I don't think that case can happen in AIE2, but who knows.

However, I'm now thinking about a case like this:

%0:vec256 = VLD $crSat = ... %1:vec512 = VUNPACK %0, implicit $crSat

There we cannot advance the VUNPACK, because it would then use a different value for $crSat.

Yes, this could be a case if we extend for VUPS case...

The best, in this case, is to iterate over all uses and defs, like:

bool isUseOf(const MachineInstr &MI, const MachineInstr &Def) { for (auto &Defs : Def.all_defs()) { for (auto &MIUse : MI.all_uses()) { if (MIUse.isReg() && Defs.getReg() == MIUse.getReg()) return true; } } return false; }

Oh alright, it's VPACK that uses crSat, not VUNPACK. Still, the latter could use crUnpackSign, so we can end up with the same problem. I think we need to use all_uses and all_defs like you proposed.

llvm/lib/Target/AIE/AIECombinerHelper.cpp

llvm/test/CodeGen/AIE/aie2/GlobalISel/inst-select-no-combine-vldb_unpack.mir

F-Stuckmann · 2024-11-21T13:26:09Z

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp

@@ -1628,6 +1633,9 @@ bool AIE2InstructionSelector::selectG_AIE_LOAD_UNPACK(
  Register DstReg = UNPACKI.getOperand(0).getReg();
  Register SignReg = UNPACKI.getOperand(3).getReg();

+  if (ShouldAntecipate)


should it not be ShouldAnticipate ?
Is there a more descriptive name for it? Like CanJoinLoadAndUse ?

I changed the name and also included a comment.

gbossu

LGTM, but before merging, we need to be a bit more careful about implicit uses/defs I think. Otherwise I think it's nice and concise code :)

krishnamtibrewala · 2024-11-22T03:47:25Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+  auto UnsafeToMoveBefore = [&](const MachineInstr &MI) {
+    return (isUseOf(Dest, MI));
+  };
+  return none_of(InstrRange, UnsafeToMoveBefore);


Nice way of using none_of 🚀

andcarminati · 2024-11-22T07:45:02Z

Hi @gbossu, all comments were addressed. Thank you for the review ;-).

gbossu · 2024-11-26T10:12:16Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

-    for (auto &MIUse : MI.uses()) {
+bool isUseOf(const MachineInstr &MI, const MachineInstr &Def) {
+  for (auto &Defs : Def.all_defs()) {
+    for (auto &MIUse : MI.all_uses()) {


Could you add a test to check that the VPACK/VUNPACK isn't advanced when one of its implicit inputs is defined in between the load/store and the op?
E.g.

%0 = VLD ... VST ... ; prevent delaying VLD $crSign = ... %1 = VPACK %0, implicit $crSign

I included a test for VUNPACK with a control register write in between. I am also looking explicitly to G_INTRINSIC_W_SIDE_EFFECTS when we try to advance an instruction.

If we can't delay VLD, we can try to antecipate UNPACK. This approach can be extended to other selection combiners.

andcarminati · 2024-11-26T15:26:11Z

After this change:

  for (auto &Defs : Def.all_defs()) {
    for (auto &MIUse : MI.all_uses()) {

No side-effects were observed in QoR.

gbossu · 2024-11-26T16:16:12Z

llvm/test/CodeGen/AIE/aie2/GlobalISel/inst-select-no-combine-vldb_unpack.mir

+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:er = COPY $r0
+    ; CHECK-NEXT: [[COPY2:%[0-9]+]]:em = COPY [[COPY1]]
+    ; CHECK-NEXT: [[VLD_pstm_pseudo:%[0-9]+]]:vec256, [[VLD_pstm_pseudo1:%[0-9]+]]:ep = VLD_pstm_pseudo [[COPY]], [[COPY2]] :: (load (<32 x s8>))
+    ; CHECK-NEXT: $crunpacksign = COPY [[COPY1]]
    ; CHECK-NEXT: [[VUNPACK_S16_S8_:%[0-9]+]]:vec512 = VUNPACK_S16_S8 [[VLD_pstm_pseudo]]


Super-nit: we are writing to crSign, but VUNPACK_S16_S8 does not read it. If we get smarter, I guess we will be able to combine. I guess what we should really check is VST + PACK with a set_crsat() in between.

Sure it could be improved even more. When we are combining/selecting, this csr write in-between is just an intrinsic call with a constant operand representing the register. We can parse this call against the registers that are consumed by the final instruction (we need a mapping for this). About the test, VUNPACK_S16_S8 uses indeed the crunpacksign, but if we change the register, the result will be the same.

As this PR solves the problems of the tracked benchmarks, we can keep it as is and look to set_csr() in the future if we see this as a blocker in some benchmarks.

gbossu

LGTM!

andcarminati requested review from abhinay-anubola, abnikant, gbossu, khallouh, konstantinschwarz, martien-de-jong, SagarMaheshwari99 and stephenneuendorffer as code owners November 20, 2024 11:09

gbossu reviewed Nov 20, 2024

View reviewed changes

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

gbossu reviewed Nov 20, 2024

View reviewed changes

llvm/lib/Target/AIE/AIECombinerHelper.cpp Show resolved Hide resolved

gbossu reviewed Nov 20, 2024

View reviewed changes

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

gbossu reviewed Nov 20, 2024

View reviewed changes

llvm/test/CodeGen/AIE/aie2/GlobalISel/inst-select-no-combine-vldb_unpack.mir Outdated Show resolved Hide resolved

andcarminati force-pushed the andreu.upcombining.vld.unpack branch from fcdfcd4 to 20b677f Compare November 20, 2024 14:04

F-Stuckmann reviewed Nov 21, 2024

View reviewed changes

andcarminati force-pushed the andreu.upcombining.vld.unpack branch from 20b677f to 1bb9385 Compare November 21, 2024 13:41

gbossu reviewed Nov 21, 2024

View reviewed changes

krishnamtibrewala reviewed Nov 22, 2024

View reviewed changes

andcarminati force-pushed the andreu.upcombining.vld.unpack branch from 1bb9385 to 11f9a74 Compare November 22, 2024 07:42

andcarminati force-pushed the andreu.upcombining.vld.unpack branch from 11f9a74 to c00687b Compare November 22, 2024 14:26

gbossu reviewed Nov 26, 2024

View reviewed changes

[AIE2] Enable up-combining for VLD + UNPACK

2a76196

If we can't delay VLD, we can try to antecipate UNPACK. This approach can be extended to other selection combiners.

andcarminati force-pushed the andreu.upcombining.vld.unpack branch from c00687b to 2a76196 Compare November 26, 2024 15:21

gbossu reviewed Nov 26, 2024

View reviewed changes

gbossu approved these changes Nov 26, 2024

View reviewed changes

andcarminati merged commit aa267e3 into aie-public Nov 27, 2024
8 checks passed

andcarminati deleted the andreu.upcombining.vld.unpack branch November 27, 2024 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2] Enable up-combining for VLD + UNPACK #241

[AIE2] Enable up-combining for VLD + UNPACK #241

andcarminati commented Nov 20, 2024

andcarminati commented Nov 20, 2024

gbossu Nov 20, 2024

andcarminati Nov 20, 2024

gbossu Nov 21, 2024

andcarminati Nov 21, 2024

andcarminati Nov 21, 2024

gbossu Nov 21, 2024

F-Stuckmann Nov 21, 2024

andcarminati Nov 21, 2024

gbossu left a comment

krishnamtibrewala Nov 22, 2024

andcarminati commented Nov 22, 2024

gbossu Nov 26, 2024

andcarminati Nov 26, 2024

andcarminati commented Nov 26, 2024

gbossu Nov 26, 2024

andcarminati Nov 27, 2024

gbossu left a comment

[AIE2] Enable up-combining for VLD + UNPACK #241

[AIE2] Enable up-combining for VLD + UNPACK #241

Conversation

andcarminati commented Nov 20, 2024

andcarminati commented Nov 20, 2024

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

gbossu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andcarminati commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andcarminati commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbossu left a comment

Choose a reason for hiding this comment