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

[Sam Parker via Phabricator via llvm-commits]

samparker marked an inline comment as done.
samparker added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3841
+                                       ExtVT, LoadN->getAddressSpace(),
+                                       ShAmt / 8))
+    return false;
----------------
gchatelet wrote:
> 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?
I'm not sure about the assert - as I remember, this function should just return false for anything that wouldn't be a legal load, whereas it's perfectly legal to have a shift of, say, 48 somewhere in the DAG. Other than that, looks like a good fix to me.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D41350