[PATCH] D20485: [esan] Add working set base runtime library

[Mike Aizatsky via llvm-commits]

aizatsky added inline comments.

================
Comment at: lib/esan/esan.cpp:120
@@ -116,2 +119,3 @@
   if (WhichTool == ESAN_CacheFrag)
     Mapping.initialize(2); // 4B:1B, so 4 to 1 == >>2.
+  else if (WhichTool == ESAN_WorkingSet)
----------------
please introduce constants for these 2 & 6.

================
Comment at: lib/esan/working_set.cpp:34
@@ +33,3 @@
+// See the shadow byte layout description above.
+static const u32 TotalWorkingSetBitIdx = 7;
+static const u32 CurWorkingSetBitIdx = 0;
----------------
bruening wrote:
> aizatsky wrote:
> > Instead of bit fiddling, do you think a struct with bit fields would look better? 
> Hmm, the issue is that all of the code (not just this CL, but code that uses the other 6 bits) operates on the bits in a way that is not very amenable to using separate named fields: samples at each bit level accumulate into the bit to the left, we would still be creating masks for word-level and other operations (rather than hoping the compiler turns several field references into a single mask), etc.  While a struct might help with this top-level documentation, I don't think it would easily help with the actual code, and the declared field names would not be referenced.
> 
> I know you haven't seen any other code operating on the shadow values yet, so how about this: let's hold that thought, and once you see some future code that operates on all the bits, if you have an idea for how to use named bitfields in the code we can revisit at that point?
SG let's see it coming.

If bitfields wouldn't work, it might be beneficial to extract lower-level shadow memory interface that contains all bit-fiddling. Not sure how similar would it be to processRangeAccessWorkingSet, but in the future I'd like to see these low-level details more contained and better structured.

================
Comment at: lib/esan/working_set.cpp:53
@@ +52,3 @@
+  while (I < NumLines && (uptr)Shadow % 4 != 0) {
+    if ((*Shadow & ShadowAccessedVal) != ShadowAccessedVal)
+      *Shadow |= ShadowAccessedVal;
----------------
I don't really see a reason to do an if() check here. Why not blindly "*Shadow |= ShadowAccessedVal"? Is it really faster? I would guess that having a branch is not very good for performance?


http://reviews.llvm.org/D20485