[PATCH] D41350: [DAGCombine] Improve ReduceLoadWidth for SRL

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.
Herald added subscribers: ecnelises, wuzish.
Herald added a project: LLVM.


================
Comment at: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3841
+                                       ExtVT, LoadN->getAddressSpace(),
+                                       ShAmt / 8))
+    return false;
----------------
I stumbled upon this code while debugging a regression in D81379.
AFAICT `ShAmt` (ShiftAmount?) can be a non power of two, especially when using the sanitizers.
For instance, `ShAmt` can be `48`, it is a round number of byte but not a valid alignment `48/8==6`.

I'm not quite sure what is the expected fix so I'm commenting here to get some feedback.
It seems to me that it should be the natural alignment for `ExtVT` possibly constrained by the original `LoadN` alignment.
Something along those lines:
```
  if (ShAmt) {
    assert(ShAmt % 8 == 0 && "ShAmt is byte offset");
    const unsigned ByteShAmt = ShAmt / 8;
    const Align LDSTAlign = LDST->getAlign();
    const Align NarrowAlign = commonAlignment(LDSTAlign, ByteShAmt);
    if (!TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), MemVT,
                                LDST->getAddressSpace(), NarrowAlign.value(),
                                LDST->getMemOperand()->getFlags()))
      return false;
  }
```

Would that make sense?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D41350