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

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.
filcab added a comment.

I would prefer having a working tool before adding the fast-paths.

Thanks for woking on this,

Filipe


================
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,
----------------
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.

================
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:
> 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.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:71
@@ +70,3 @@
+static const uint64_t ShadowOffs[4] = { // Indexed by scale
+  0x0000130000000000ull,
+  0x0000220000000000ull,
----------------
aizatsky wrote:
> bruening wrote:
> > aizatsky wrote:
> > > uh-oh. These are really magical constants copied.
> > > Unfortunately there's no proper way to share code between runtime / llvm. I talked with our cube and the idea is to introduce a separate header (e.g. esan_compile_interface.h) and put all shareable constants there. At least that would make it clear what you are sharing. At best - you can manually synchronize the file by copying. WDYT?
> > Yes, this lack of ability to share is something I was surprised at in the other sanitizers.  I talked to kcc about it early on when starting this project and it sounded like copying a handful of constants was something everyone was willing to live with and that they did not plan to push for any better approach, so I followed suit here.
> > 
> > Sure, a separate header sounds good.  Is there a plan to do this for the other sanitizers?
> > 
> > I assume the header would go into include/llvm/Transforms/ and should probably follow the CamelCase naming.
> > Sure, a separate header sounds good. Is there a plan to do this for the other sanitizers?
> 
> Everyone agrees that the way it is now is very painful and we need to try this. If a separate header works for you - we would definitely steer others in the same direction.
> 
> > I assume the header would go into include/llvm/Transforms/ and should probably follow the CamelCase naming.
> 
> Yes. Even if you don't decide to copy it to runtime, it would clearly document all inter-dependencies and will make updating easier.
> Yes. Even if you don't decide to copy it to runtime, it would clearly document all inter-dependencies and will make updating easier.

I don't like that half-way thing. Please, if you're making it a header signalizing that the stuff there is shared, then do it on both places, not just one.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:99
@@ -86,1 +98,3 @@
+      : FunctionPass(ID), Options(OverrideOptionsFromCL(Opts)),
+        ShadowScale(3) {}
   const char *getPassName() const override;
----------------
`kDefaultCacheFragToolShadowScale` or something.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:207
@@ +206,3 @@
+  if (Options.ToolType == EfficiencySanitizerOptions::ESAN_WorkingSet)
+    ShadowScale = 6; // 64B:1B mapping, so 64:1.
+
----------------
I'd prefer to construct the object in an invalid state and have `doInitialization` always set `ShadowScale`, etc.
That way we'd only need to look at this if we're experimenting with different shadow scales, etc. Instead of having to track down the initial setting for the default tool, or the other tools.
We'd have much better code locality this way.

P.S: `s/6/kDefaultWorkingSetToolShadowScale/` or something.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:309
@@ +308,3 @@
+  if (Alignment == 0)
+    Alignment = DL.getPrefTypeAlignment(OrigTy);
+
----------------
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.

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

================
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.
----------------
Don't you need an atomic load? At the very least, if the original instruction was atomic?

================
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);
----------------
Same for atomic store.


http://reviews.llvm.org/D20483