[PATCH] D95543: [GVN] Clobber partially aliased loads.

Mon Apr 12 10:59:43 PDT 2021

dfukalov marked 2 inline comments as done.
dfukalov added inline comments.

================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:518
-      // at the clobbering load directly, it doesn't know about any
-      // phi translation that may have happened along the way.
-
----------------
nikic wrote:
> Do you have any information on why this is not a concern anymore? The comment was introduced in https://github.com/llvm/llvm-project/commit/a471751c24324e7ba6ac5c612dbedb16c644fc44, unfortunately without a test case :(
Yes, I saw this change and because of this comment added VisitedPhiBBs check when [[ https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Analysis/BasicAliasAnalysis.cpp$1114 | we return PartialAlias with offset]] in BasicAAResult::aliasGEP.

================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:522
+            ClobberOffset = None;
           return MemDepResult::getClobber(Inst);
+        }
----------------
nikic wrote:
> Say we have a load from X, then a load from PartialAlias of X without offset, then another load from X. I think after your change, we will no longer forward from the first load, because we'll return a clobber for the PartialAlias in between, even though it cannot be used.
> 
> Can you please test this situation? Maybe we should not return a clobber if no offset is available?
Yes, you're right there is no reason to return here PartialAlias if we don't know the offset.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95543