[RFC][AIE2][AA] Disambiguate pointers updated by intrinsics #139

andcarminati · 2024-07-30T11:08:19Z

This is RFC. Soon we will need more AA support, and this PR tries to clear a bit the way.

Now we can disambiguate some cases where aie2_add_2d and aie2_add_3d are used to update pointers.

No QoR failures.
Gains for Reduce* family up to 50% (instruction count).
Average gain is ~2% (~350 benchmarks).

Updated to include post-RA AA support.

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

llvm/test/CodeGen/AIE/alias-analysis-pointer-desambiguation.ll

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

gbossu · 2024-08-19T14:25:08Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+    } else {
+      if (auto *PHI = dyn_cast<PHINode>(V)) {
+        // Reached final destination.
+        return {CountIntrinsicCall, ArgSet};


Should we check that this PHI node corresponds to PhiA/PhiB in aliasAIEIntrinsic?

We can assume the same as the comment below

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

llvm/test/CodeGen/AIE/alias-analysis-pointer-desambiguation.ll

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

gbossu · 2024-08-23T14:03:56Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+
+// We can reach a pointer definition starting from a counter.
+static bool isDirectReachable(SmallVector<const Value *, 5> CounterArgSet,
+                              const Value *V) {


Nit: maybe

static bool isAddressingIntrinsicOutput(ArrayRef<const Value*> AddressingOutputs, const Value *InitialAddress);

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

gbossu · 2024-08-26T12:42:19Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+    }
+    // This information is crucial to calculate the number
+    // of pointer updates before TargetPointer.
+    if (LastUpdate == TargetPointer)


Should we actually check skipCast(LastUpdate) == TargetPointer?

Could you maybe add a test where there is a pointer cast right before the addressing intrinsic?

Done! @with_addrspace_cast.

gbossu · 2024-08-26T12:52:11Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+    return true;
+  }
+
+  bool notOverlap(const IntrinsicChainInfo &Other) const {


Super-nit: I'd find overlaps() clearer, i.e. avoiding double negations.

I guess that would have to be mayOverlap()

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

martien-de-jong · 2024-08-26T13:38:39Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+};
+
+struct IntrinsicChainInfo {
+  // Calls to an specific point.


nit: upto a specific

martien-de-jong · 2024-08-26T13:41:07Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+struct IntrinsicChainInfo {
+  // Calls to an specific point.
+  unsigned IntrinsicCalls = 0;
+  // Total calls accross the loop.


nit: across

martien-de-jong · 2024-08-26T13:43:16Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+
+struct IntrinsicArgsInfo {
+  SmallVector<const Value *, 5> ArgSet;
+  SmallVector<const Value *, 5> CounterArgSet;


I think they are ordered. 'Set' suggests they are not.

martien-de-jong · 2024-08-26T13:50:00Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+    return true;
+  }
+
+  bool notOverlap(const IntrinsicChainInfo &Other) const {


I guess that would have to be mayOverlap()

martien-de-jong · 2024-08-26T13:53:38Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+  Intrinsic::ID ID = IntrinsicCall->getIntrinsicID();
+  IntrinsicArgsInfo Args;
+  unsigned InPtrIdx;
+  isAIEPtrAddIntrinsic(ID, InPtrIdx);


This looks unused?

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

martien-de-jong · 2024-08-26T14:05:34Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+      // the same value
+      LastUpdate = GEP->getPointerOperand();
+    } else if (auto *PHI = dyn_cast<PHINode>(LastUpdate)) {
+      // Reached final destination.


I would make this the first case in the if-then-elseif ladder.

I changed a bit the code and the current position matters, because now the intrinsic part is in the else block.

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

martien-de-jong · 2024-08-26T14:41:08Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+    if (!ValA)
+      return AliasResult::MayAlias;
+
+    const Value *BaseA = getUnderlyingObjectAIE(ValA);


factor into std::optional<ValueBasePair> lambda?

martien-de-jong · 2024-08-26T14:48:19Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+  // Non uniform address update accross the loop.
+  // * one can have more updates or,
+  // * non-uniform updates were found.
+  if (!ATracking || !BTracking)


I would check ATracking before trying to compute BTracking.

gbossu · 2024-08-27T13:07:55Z

llvm/test/CodeGen/AIE/alias-analysis-pointer-desambiguation.ll

+  %10 = extractvalue { ptr, i20 } %8, 0
+  load i8, ptr %10
+
+  %12 = tail call { ptr, i20 } @llvm.aie2.add.2d(ptr %7, i20 0, i20 1, i20 2, i20 %6)


Is there any way to get the alias info for %7? Maybe by inserting a store to %7 on top of the one for %ptrcast?

I'm suspecting that the alias info will be wrong for %7, because in the code below, I think that LastUpdate will point to %ptrcast after visiting the instrinsic, which is different from %7, so the update of LastCalls will not trigger.

%ptrcast = addrspacecast ptr %7 to ptr addrspace(3) %12 = tail call { ptr, i20 } @llvm.aie2.add.2d(ptr %7, i20 0, i20 1, i20 2, i20 %6)

The cast situation is handled by the @with_addrspace_cast_phi test.

gbossu · 2024-08-28T13:16:06Z

llvm/unittests/Target/AIE/VirtUnrollAliasAnalysisTest.cpp

+  // This case is interesting because the first store from the 3rd iteration
+  // will alias with the 3rd load of the second iteration since we are not
+  // updating the pointer after the last load and store (post. inc.).
+  ASSERT_TRUE(AIE::aliasAcrossVirtualUnrolls(Store1, Load3, 3, 2) ==


Nit: EXPECT_TRUE is better because it doesn't stop the test execution at the first failure.

Interesting, I was not aware! I think I will leave as is this time to not interrupt CI again ;-).

I considered this! Already updated.

gbossu

LGTM, great work!

martien-de-jong · 2024-08-28T13:37:41Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+      if (LastCalls) {
+        ChainInfo.IntrinsicCalls = ChainInfo.TotalIntrinsicCalls - *LastCalls;
+        return ChainInfo;
+      } else {


no else after return?

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

Now we can disambiguate some cases where aie2_add_2d and aie2_add_3d are used to update pointers.

Now we can check instructions across virtual iterations.

martien-de-jong · 2024-09-02T15:41:51Z

llvm/lib/Target/AIE/AIEBaseAliasAnalysis.cpp

+}
+
+/// This gives all indexes counter operands.
+static SmallVector<unsigned, 5> getAddIntrinsicCounterOps(Intrinsic::ID ID) {


nit: it looks as if getAddIntriscOps is complementary to getAddIntrinsicCounterOps, so can be computed as allInputOperands - getAddIntrinsicCounterOps

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

andcarminati force-pushed the andreu.aa.intrinsic.dis branch from 7f688c0 to 2874f8d Compare July 31, 2024 12:08

andcarminati commented Jul 31, 2024

View reviewed changes

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

andcarminati force-pushed the andreu.aa.intrinsic.dis branch from 2874f8d to 48cfc8f Compare August 1, 2024 09:02

martien-de-jong reviewed Aug 1, 2024

View reviewed changes

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

martien-de-jong reviewed Aug 1, 2024

View reviewed changes

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

martien-de-jong reviewed Aug 1, 2024

View reviewed changes

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

martien-de-jong reviewed Aug 1, 2024

View reviewed changes

llvm/test/CodeGen/AIE/alias-analysis-pointer-desambiguation.ll Outdated Show resolved Hide resolved

martien-de-jong reviewed Aug 1, 2024

View reviewed changes

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

andcarminati force-pushed the andreu.aa.intrinsic.dis branch from 48cfc8f to e6fa65d Compare August 7, 2024 13:28

andcarminati marked this pull request as draft August 7, 2024 13:53

andcarminati force-pushed the andreu.aa.intrinsic.dis branch 2 times, most recently from 66a8326 to aab392c Compare August 16, 2024 08:54

gbossu reviewed Aug 19, 2024

View reviewed changes

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

gbossu reviewed Aug 19, 2024

View reviewed changes

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

gbossu reviewed Aug 19, 2024

View reviewed changes

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

gbossu reviewed Aug 19, 2024

View reviewed changes

llvm/test/CodeGen/AIE/alias-analysis-pointer-desambiguation.ll Outdated Show resolved Hide resolved

andcarminati force-pushed the andreu.aa.intrinsic.dis branch from aab392c to d3a0a58 Compare August 20, 2024 12:39

gbossu reviewed Aug 21, 2024

View reviewed changes

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

gbossu reviewed Aug 21, 2024

View reviewed changes

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

gbossu reviewed Aug 21, 2024

View reviewed changes

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

gbossu reviewed Aug 21, 2024

View reviewed changes

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

andcarminati commented Aug 23, 2024

View reviewed changes

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

gbossu reviewed Aug 23, 2024

View reviewed changes

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

gbossu reviewed Aug 23, 2024

View reviewed changes

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

gbossu reviewed Aug 23, 2024

View reviewed changes

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

andcarminati force-pushed the andreu.aa.intrinsic.dis branch 2 times, most recently from eb57536 to c12d012 Compare August 26, 2024 07:16

gbossu reviewed Aug 26, 2024

View reviewed changes

andcarminati marked this pull request as ready for review August 26, 2024 13:30

martien-de-jong reviewed Aug 26, 2024

View reviewed changes

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

martien-de-jong reviewed Aug 26, 2024

View reviewed changes

andcarminati force-pushed the andreu.aa.intrinsic.dis branch from c12d012 to 7ca270d Compare August 27, 2024 11:59

gbossu reviewed Aug 27, 2024

View reviewed changes

andcarminati force-pushed the andreu.aa.intrinsic.dis branch 2 times, most recently from 2b9be93 to d421e71 Compare August 28, 2024 13:05

gbossu reviewed Aug 28, 2024

View reviewed changes

gbossu previously approved these changes Aug 28, 2024

View reviewed changes

martien-de-jong reviewed Aug 28, 2024

View reviewed changes

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

andcarminati added 2 commits August 28, 2024 15:40

[AIE2][AA] Disambiguate pointers updated by intrinsics

b0c83c7

Now we can disambiguate some cases where aie2_add_2d and aie2_add_3d are used to update pointers.

[AIE2][AA] Add support for virtual unrolling AA

1312139

Now we can check instructions across virtual iterations.

andcarminati dismissed gbossu’s stale review via 1312139 August 28, 2024 14:42

andcarminati force-pushed the andreu.aa.intrinsic.dis branch from d421e71 to 1312139 Compare August 28, 2024 14:42

gbossu approved these changes Aug 28, 2024

View reviewed changes

martien-de-jong approved these changes Sep 2, 2024

View reviewed changes

andcarminati merged commit 5492f45 into aie-public Sep 2, 2024
8 checks passed

andcarminati deleted the andreu.aa.intrinsic.dis branch September 2, 2024 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][AIE2][AA] Disambiguate pointers updated by intrinsics #139

[RFC][AIE2][AA] Disambiguate pointers updated by intrinsics #139

andcarminati commented Jul 30, 2024 •

edited

Loading

gbossu Aug 19, 2024

andcarminati Aug 22, 2024

gbossu Aug 23, 2024

gbossu Aug 26, 2024

gbossu Aug 26, 2024

andcarminati Aug 27, 2024

gbossu Aug 26, 2024

martien-de-jong Aug 26, 2024

martien-de-jong Aug 26, 2024

martien-de-jong Aug 26, 2024

martien-de-jong Aug 26, 2024

martien-de-jong Aug 26, 2024

martien-de-jong Aug 26, 2024

martien-de-jong Aug 26, 2024

andcarminati Aug 27, 2024

martien-de-jong Aug 26, 2024 •

edited

Loading

martien-de-jong Aug 26, 2024

gbossu Aug 27, 2024

andcarminati Aug 28, 2024

gbossu Aug 28, 2024

andcarminati Aug 28, 2024 •

edited

Loading

andcarminati Aug 28, 2024

gbossu left a comment

martien-de-jong Aug 28, 2024

martien-de-jong Sep 2, 2024

[RFC][AIE2][AA] Disambiguate pointers updated by intrinsics #139

[RFC][AIE2][AA] Disambiguate pointers updated by intrinsics #139

Conversation

andcarminati commented Jul 30, 2024 • 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

martien-de-jong Aug 26, 2024 • 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

andcarminati Aug 28, 2024 • edited Loading

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

Choose a reason for hiding this comment

andcarminati commented Jul 30, 2024 •

edited

Loading

martien-de-jong Aug 26, 2024 •

edited

Loading

andcarminati Aug 28, 2024 •

edited

Loading