[PATCH] D89149: [SelectionDAG] Fix alias checking with potential later stack reuse

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:116
-  // can infer there is no alias.
-  if (auto *A = dyn_cast<FrameIndexSDNode>(BasePtr0.getBase()))
-    if (auto *B = dyn_cast<FrameIndexSDNode>(BasePtr1.getBase())) {
----------------
niravd wrote:
> Removing this would be a bit of a shame as it has some nice improvements in other backends. (Also, is it only showing up in BPF or can you replicate this on X86/ARM?
> 
> I don't have the best knowledge on FrameInfo structures, so we should probably loop in someone who gets that better, but we should see if we can extract disjointed information out of the frame info. 
> 
> At the very least, we should be able to retain that they can't alias if exactly one is fixed. However, the vast majority of improvements from this was catching disjointness of two separate allocations to see if we can keep this. 
> 
> 
> 
> 
> 
> 
> Removing this would be a bit of a shame as it has some nice improvements in other backends. (Also, is it only showing up in BPF or can you replicate this on X86/ARM?

I did not check ARM, but the issue did not show up on X86. X86 implemented enableMachineScheduler() returning true, and this ensures
Source scheduler is used for selection dag,
createDefaultScheduler():
   ...
    if (OptLevel == CodeGenOpt::None ||
        (ST.enableMachineScheduler() &&   ST.enableMachineSchedDefaultSched()) ||
        TLI->getSchedulingPreference() == Sched::Source)
      return createSourceListDAGScheduler(IS, OptLevel);
 ...

For Source dag scheduler, if there are multiple choices, it perfers to follow lexicographic order. So this won't be an issue for X86.

ARM and AArch64 has similar override so it also uses Source dag scheduler and it won't have issue either.

I have a similar patch for BPF backend to also use Source scheduler:
  https://reviews.llvm.org/D88525
But I feel that does not fix the root cause, so hence this patch. But Source scheduler should fix the issue for disjoint allocations since even these allocations are reused, Source schedule ensures early load/stores happen before later load/stores...

> 
> I don't have the best knowledge on FrameInfo structures, so we should probably loop in someone who gets that better, but we should see if we can extract disjointed information out of the frame info. 
> 
> At the very least, we should be able to retain that they can't alias if exactly one is fixed. However, the vast majority of improvements from this was catching disjointness of two separate allocations to see if we can keep this. 

Yes, the majority cases should be from two separate allocations. But theoretically separate allocations can be reused as long as lifetime start/end are honored for allocations, right? There are some x86 tests failed with my patch, I could spend some time on these failed tests to find patterns so we can try to maintain good cases...



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:130
+
+  if ((IdxCheck || (IsFI0 != IsFI1) || (IsGV0 != IsGV1) || (IsCV0 != IsCV1)) &&
       (IsFI0 || IsGV0 || IsCV0) && (IsFI1 || IsGV1 || IsCV1)) {
----------------
niravd wrote:
> Can you change this so we check the two Index value match no matter what if we show non-aliasing? We really should check the both base and Index if we're going to show non-aliasing.
> 
> if (((BasePtr0.getIndex() == BasePtr1.getIndex()) &&
>      ((IsFI0 != IsFI1) || (IsGV0 != IsGV1) || (IsCV0 != IsCV1)) &&
Yes, I can do this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89149/new/

https://reviews.llvm.org/D89149