[PATCH] D87538: [VectorCombine] Don't vectorize scalar load under asan/hwasan/memtag/tsan

Mon Sep 14 13:53:56 PDT 2020

MaskRay added inline comments.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4414
 bool llvm::mustSuppressSpeculation(const LoadInst &LI) {
-  if (!LI.isUnordered())
+  if (!LI.isSimple())
     return true;
----------------
spatel wrote:
> Definitely get a 2nd reviewer opinion if we are making changes here (I don't know enough about this)...
> But if I'm seeing it correctly, isSimple() is not a superset of isUnordered(). Ie, a load can have AtomicOrdering::Unordered but still be simple?
isSimple is a subset of isUnordered. An AtomicOrdering::Unordered load is `isUnordered` but not `isSimple`.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4422
+         F.hasFnAttribute(Attribute::SanitizeHWAddress) ||
+         F.hasFnAttribute(Attribute::SanitizeMemTag);
 }
----------------
eugenis wrote:
> MemTag allows extending memory access up to the next 16-byte granule. Memtag is a production thing, so it's important that we do not lose performance there when possible.
Do you suggest that we should remove SanitizeMemTag here and exclude SanitizeMemTag from VectorCombine.cpp instead? 

(VectorCombine.cpp can check alignment and allow SanitizeMemTag as an optimization but that will take more code that I don't want to take risk.

I am posting the patch to fix a tsan spurious report.
)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87538