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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers 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 &&
----------------
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.


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

https://reviews.llvm.org/D75238