[PATCH] D41350: [DAGCombine] Improve ReduceLoadWidth for SRL
Sam Parker via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 10 02:08:57 PDT 2020
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
More information about the llvm-commits
mailing list