[PATCH] D37698: Add target callback doMemOpsHaveSameBasePtr

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

In https://reviews.llvm.org/D37698#869935, @rampitec wrote:

> 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?


While you could just as well move the class declaration into a header an extra function argument is probably easiest.

> 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?

Indeed, that sounds like the best/easiest solution so far!

> 
> 
>> - 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