[PATCH] D140046: [PowerPC] Fix up memory ordering after combining BV to a load

Thu Dec 15 05:39:08 PST 2022

nemanjai added a comment.

In D140046#3996275 <https://reviews.llvm.org/D140046#3996275>, @RKSimon wrote:

> LGTM, although its annoying that you can't use SelectionDAG::areNonVolatileConsecutiveLoads somehow.
>
> One minor - nothing to do with this patch, but something that probably should be addressed if you're in the vicinity.

I think we just missed `SelectionDAG::areNonVolatileConsecutiveLoads` in the initial implementation. We should be able to use that, but I can refactor that in a follow-up.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14262

+  SmallPtrSet<LoadSDNode *, 4> InputLoads;
   for (int i = 1, e = N->getNumOperands(); i < e; ++i) {
----------------
efriedma wrote:
> Do we really need a SmallPtrSet here?  We check each load exactly once, so we should be able to just use a SmallVector.
> 
> Iterating over a SmallPtrSet like this might lead to non-determinism.
Sounds good. I'll change it to `SmallVector`.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14276
     LoadSDNode *LD1 = dyn_cast<LoadSDNode>(PreviousInput);
     LoadSDNode *LD2 = dyn_cast<LoadSDNode>(NextInput);

----------------
RKSimon wrote:
> You're immediately dereferencing dyn_cast<> - these can probably be cast<>? 
Sure, I can change that while I'm here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140046