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

[Dave Green via Phabricator via llvm-commits]

dmgreen marked 2 inline comments as done.
dmgreen added a comment.

In D75238#1896424 <https://reviews.llvm.org/D75238#1896424>, @yabinc wrote:

> Thanks for the fix! I have verified that it fixes tests broken by using AA.


Brilliant, thanks!



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21134
+      SrcValOffset0 % *MUC0.NumBytes == 0 &&
+      SrcValOffset1 % *MUC0.NumBytes == 0) {
     int64_t OffAlign0 = SrcValOffset0 % OrigAlignment0;
----------------
nickdesaulniers wrote:
> srhines wrote:
> > nickdesaulniers wrote:
> > > Probably should hoist `MUC0.NumBytes` and `MUC1.NumBytes` above the if into their own locals, like the offsets and alignments; would help make this condition and block more readable.
> > > 
> > > Also, was mod'ing `SrcValueOffset1` by `*MUC0.NumBytes` intentional? Should it be `*MUC1.NumBytes`?
> > `*MUC0.NumBytes == *MUC1.NumBytes` from earlier in this conditional.
> Still...
Yeah, it was intentional from the fact that they are equal. I can change it though, it sounds cleaner that way.


================
Comment at: llvm/test/CodeGen/ARM/memset-align.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=thumbv8-unknown-linux-android10000 -o - | FileCheck %s
+
----------------
srhines wrote:
> nickdesaulniers wrote:
> > What is going on with this target triple? LOL
> The "10000" changes the Android target API level to 10000 (which basically means everything is available). I think it is safe to remove it for the purpose of this test, but @yabinc 's example used this in the command line because of how it works on the platform build for Android.
Righteo. I'll changed it to.. thumbv8-unknown-linux-android. Let me know if something else would be better.


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

https://reviews.llvm.org/D75238