[PATCH] D20483: [esan] EfficiencySanitizer working set tool fastpath

[Derek Bruening via llvm-commits]

bruening marked 3 inline comments as done.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:70
@@ +69,3 @@
+static const uint64_t ShadowMask = 0x00000fffffffffffull;
+static const uint64_t ShadowOffs[4] = { // Indexed by scale
+  0x0000130000000000ull,
----------------
filcab wrote:
> filcab wrote:
> > Is the `4` on purpose, with three constants? Either way, I'd prefer to have no bound on the declaration and have an explicit initialization (with the last one as `0`), to explicitly show it's on purpose (or keep the size *and* add the extra `0`. If we had a lot of zeros at the end, then I'd be more ok with having the bounds + "hidden" zeros.
> I'd prefer to have the tool name (or some abbreviation if it's too long) in the shadow scales/offsets, if it's expected to change between tools. If it's the same for both the tools we have now (cache frag + working set), then I'm ok with leaving it as is (+ small comment saying it's those values for both those tools) for now.
These do not change: our general shadow mapping handles all scales we will want.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:309
@@ +308,3 @@
+  if (Alignment == 0)
+    Alignment = DL.getPrefTypeAlignment(OrigTy);
+
----------------
filcab wrote:
> I'm not sure this is appropriate.
> Shouldn't you be setting `Alignment = 1` if it's 0 (which guarantees no alignment)?
> Can loads/stores ever have an alignment that is 0?
> If they can (and do), then setting it here isn't too bad. But otherwise, I'd have an `assert(Alignment != 0)` here, and just set `Alignment` to 1 above, where you're setting it to 0, instead of setting it twice.
Are you sure that 0 means no alignment?  Is that documented somewhere?  I looked and I could not find a definitive answer to what 0 means.  The alignment of a brand-new LoadInst is 0.  Tsan and asan treat alignment of 0 as type-aligned with checks like this:

  if (Alignment == 0 || Alignment >= 8 || (Alignment % (TypeSize / 8)) == 0)

Additionally, looking around I see code like this:

lib/CodeGen/SelectionDAG/SelectionDAG.cpp:  if (Alignment == 0)
lib/CodeGen/SelectionDAG/SelectionDAG.cpp:    Alignment = getDataLayout().getPrefTypeAlignment(C->getType());

Which while it's not specifically about loads or stores, combined with the behavior of the other sanitizers, implies that 0 means the default for the platform.

If someone has a definitive answer to what 0 means, please update the docs and all the existing sanitizers.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:409
@@ +408,3 @@
+        (Alignment % (TypeSize / 8)) == 0))
+    return false;
+
----------------
filcab wrote:
> Should you have a debug printf or some statistics on skipped addresses?
See NumFastpaths.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:424
@@ +423,3 @@
+
+  Value *OldValue = IRB.CreateLoad(IRB.CreateIntToPtr(ShadowPtr, ShadowPtrTy));
+  // The AND and CMP will be turned into a TEST instruction by the compiler.
----------------
filcab wrote:
> Don't you need an atomic load? At the very least, if the original instruction was atomic?
We are explicitly fine with races accessing our shadow memory.  This is documented in the shadow interface.  I will add another comment here.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:433
@@ +432,3 @@
+  Value *NewVal = IRB.CreateOr(OldValue, ValueMask);
+  IRB.CreateStore(NewVal, IRB.CreateIntToPtr(ShadowPtr, ShadowPtrTy));
+  IRB.SetInsertPoint(I);
----------------
filcab wrote:
> Same for atomic store.
Ditto.


http://reviews.llvm.org/D20483