[PATCH] D75238: [DAGCombine] Fix alias analysis for unaligned accesses

[Dave Green via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21131
+  int64_t Size0 = MUC0.NumBytes.hasValue() ? *MUC0.NumBytes : -1;
+  int64_t Size1 = MUC1.NumBytes.hasValue() ? *MUC1.NumBytes : -1;
   if (OrigAlignment0 == OrigAlignment1 && SrcValOffset0 != SrcValOffset1 &&
----------------
nickdesaulniers wrote:
> The hunk you modify on L21156-L21157 highlights was looks like maybe a pre-existing incorrect usage of `Optional::operator*`; there's no check that `MUC0.NumBytes` `hasValue` before making use of the value (which may not exist).  In particular, I'm concerned what the value of `-1` might do in those expressions.
> 
> Or you could do:
> 
> ```
> auto& Size0 = MUC0.NumBytes;
> auto& Size1 = MUC1.NumBytes;
> 
> if ( ... && Size0.hasValue() && Size0.hasValue() && ...)
>   foo(*Size0, *Size1)
> ```
> Though I'm not sure whether the lower hunk should have a guard as well vs use a value of `0`.  Using `-1` for a possible value of the `Optional<int64_t>` might be problematic, though I assume the number of bytes in a "memory use characteristic" is unlikely. (I wish the "LLVM Programmers Manual" had a section on `llvm::Optional`). So maybe it's ok to keep as is, but then I really think the lower hunk should also have some kind of guard.
Good point

I chose -1 as it's the same as MemoryLocation::UnknownSize (and only didn't use that directly here as `(int64_t)MemoryLocation::UnknownSize` makes things difficult to follow with how verbose it is). That Size is not passed straight through to AA->alias below though, so might cause weirdness if there was ever a case where MUC0.MMO had a value but Size didn't.

I'll changed it to how you suggest, with the references, and added the checks below too in case.


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

https://reviews.llvm.org/D75238