[PATCH] D106447: [DAGCombine] DAGTypeLegalizer::GenWidenVectorLoads(): make use of dereferenceability knowledge

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added subscribers: aqjune, nlopes.
lebedev.ri added a comment.

CC @nlopes @aqjune - is alive2 correct for LLVM IR semantics?

In D106447#2899471 <https://reviews.llvm.org/D106447#2899471>, @sebastian-ne wrote:

> In D106447#2899456 <https://reviews.llvm.org/D106447#2899456>, @lebedev.ri wrote:
>
>> In D106447#2899434 <https://reviews.llvm.org/D106447#2899434>, @sebastian-ne wrote:
>>
>>> There were some concerns that assuming that dereferenceable bytes >= alignment is incorrect. Memory protection may work at 4-byte or even byte granularity and objects may be over-aligned in memory. In these cases, loading padding bytes does not work.
>>>
>>> Even if it can be proven that enough bytes are dereferenceable,
>>
>> This is FUD.
>>
>> …
>>
>> We can not second-guess LLVM semantics.
>> If we can tell that we are always allowed to load N bytes without trapping, then we can do that.
>> If we actually can't, then the problem is elsewhere, we shouldn't have deduced that we can.
>
> I agree with that, I misunderstand the code then.
>
>   if (LD->isSimple()) {
>     NumDereferenceableBytes = LD->getAlignment();
>     if (!LdWidth.isScalable())
>       NumDereferenceableBytes =
>           std::max<unsigned>(NumDereferenceableBytes, LdWidth / 8);
>     if (!WidenWidth.isScalable() && NumDereferenceableBytes < WidenWidth / 8 &&
>         LD->getPointerInfo().isDereferenceable(
>             WidenWidth / 8, *DAG.getContext(), DAG.getDataLayout()))
>       NumDereferenceableBytes =
>           std::max<unsigned>(NumDereferenceableBytes, WidenWidth / 8);
>   }
>
> As far as I understand this part, `NumDereferenceableBytes` is set to the (guaranteed minimum) alignment of the load.

That is true, and please note that it is already what is happening regardless of this patch, as you can see from the LHS of the diff.
Looks like that was most originally added by rL94338 <https://reviews.llvm.org/rL94338>.

Now that i look, indeed, while deref is the source of truth (https://alive2.llvm.org/ce/z/9TAAwj vs https://alive2.llvm.org/ce/z/bmKMm4),
as per the LLVM IR semantics as modelled by alive2, alignment does not guarantee dereferenceability: https://alive2.llvm.org/ce/z/-HxoLA
(CC @nlopes @aqjune)

> In some cases, where it’s ok, e.g. because the dereferenceable size is set, `NumDereferenceableBytes` is increased.
>
> So, `NumDereferenceableBytes` is always at least the alignment?
> `isDereferenceable(LD->getAlignment())` is never checked as far as I see.

Right, but that is so regardless of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106447