[PATCH] D37698: Add target callback doMemOpsHaveSameBasePtr

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In https://reviews.llvm.org/D37698#869839, @MatzeB wrote:

> - When seeing the name `doMemOpsHaveSameBasePtr` I would expect this to do exactly `BaseReg1 == BaseReg2`. Can you explain how you happen to have cases with different base pointers that still reference the same object?


That can only work if instruction has base register and offset operands. AMDGPU flat_load and flat_store instructions do not, their address operand is a single register. If you need to load two values from the same base pointer you do:

flat_load %ptr1
%ptr2 = %ptr1 + offset
flat_load %ptr2

> - Given that this callback is very specific to the selectiondag clustering mutator, maybe we better add a callback to `createLoadClusterDAGMutation`/`createStoreClusterDAGMutation` rather than in TargetInstructionInfo. That way you could also choose a better name like `shouldCluster`.

I cannot directly inherit LoadClusterMutation or StoreClusterMutation, it is defined in the MachineScheduler.cpp, and then if I move it to common interface I do not want to duplicate the code in both. I can pass a lambda/functor into create(Load|Store)ClusterDAGMutation as a last argument. Is that what you have in mind?
As an alternative I can just drop this portion of code from BaseMemOpClusterMutation::clusterNeighboringMemOps() completely:

  if (MemOpRecords[Idx].BaseReg != MemOpRecords[Idx+1].BaseReg) {
    ClusterLength = 1;
    continue;
  }

and let TII->shouldClusterMemOps() decide. Maybe it is better. Thoughts?

> - We should indeed not change the behavior for aarch64, as the mutation mainly exists there to enable formation of load/store double instructions which require the same base pointer.

Agree. I am about to close https://reviews.llvm.org/D37755.


https://reviews.llvm.org/D37698