Page MenuHomePhabricator

[esan] EfficiencySanitizer working set tool fastpath
ClosedPublic

Authored by bruening on May 20 2016, 1:17 PM.

Details

Summary

Adds fastpath instrumentation for esan's working set tool. The
instrumentation for aligned loads and stores consists of inlined writes
to shadow memory bits for each accessed cache line.

Adds a basic test for this instrumentation.

Diff Detail

Repository
rL LLVM

Event Timeline

bruening updated this revision to Diff 57976.May 20 2016, 1:17 PM
bruening retitled this revision from to [esan] EfficiencySanitizer working set tool fastpath.
bruening updated this object.
bruening added a reviewer: aizatsky.
bruening added subscribers: llvm-commits, eugenis, kcc and 2 others.
aizatsky added inline comments.May 20 2016, 1:53 PM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
71 ↗(On Diff #57976)

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?

400 ↗(On Diff #57976)

Document what code are you generating - it is hard to read IR building.

bruening added inline comments.May 20 2016, 2:58 PM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
71 ↗(On Diff #57976)

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.

aizatsky added inline comments.May 20 2016, 3:06 PM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
71 ↗(On Diff #57976)

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.

filcab added a subscriber: filcab.May 22 2016, 3:11 AM

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

Thanks for woking on this,

Filipe

lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
70 ↗(On Diff #57976)

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.

70 ↗(On Diff #57976)

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.

71 ↗(On Diff #57976)

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.

99 ↗(On Diff #57976)

kDefaultCacheFragToolShadowScale or something.

207 ↗(On Diff #57976)

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.

309 ↗(On Diff #57976)

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.

409 ↗(On Diff #57976)

Should you have a debug printf or some statistics on skipped addresses?

424 ↗(On Diff #57976)

Don't you need an atomic load? At the very least, if the original instruction was atomic?

433 ↗(On Diff #57976)

Same for atomic store.

bruening added inline comments.May 23 2016, 9:10 AM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
71 ↗(On Diff #57976)

Thinking more about this, I'm not sure it makes sense to treat just these two shadow constants as different from all of the other shared interface constants such as all of the runtime library function names:

static const char *const EsanInitName = "__esan_init";

This includes the load and store handler names ("__esan_aligned_load8", e.g.), which are generated in EfficiencySanitizer.cpp.

The cleanest thing would be to use macros to turn esan_init into a string or a var, and include that header in both places. We'd have to remove the loop that generates esan_aligned_loadX and hardcode each one.
We'd put the ToolType enum in there too.

However, compiler-rt is not supposed to directly include an llvm header, right? If it's not allowed, and we'd rather have the loop that generates those names, and we already have a nice compiler-rt/lib/esan/esan_interface_internal.h header for the runtime, and nobody else is going to include a new llvm header, perhaps simply clearly labeling the interface (functions, tooltype, shadow consts) in a section at the top of EfficiencySanitizer.cpp is the best option.

zhaoqin added inline comments.May 23 2016, 9:27 AM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
71 ↗(On Diff #57976)

IMHO, there are two possible benefits of adding a header file here:

  1. avoid duplication
  2. clear separation/documentation about inter-dependencies between runtime.

However, since we do not include an llvm header,
we cannot avoid duplication.

And there is only one EfficiencySanitizer.cpp, it looks to me that a clear section about the duplicated part is easier to track than a separate header file.

I think the right way to share the constants between instrumentation and the runtime library is to put them in an LLVM header, install that to the build directory (under include/llvm) and locate that through llvm-config under if (COMPILER_RT_STANDALONE_BUILD).

I think the right way to share the constants between instrumentation and the runtime library is to put them in an LLVM header, install that to the build directory (under include/llvm) and locate that through llvm-config under if (COMPILER_RT_STANDALONE_BUILD).

Given that we are now talking about refactoring existing code, let's move that to a separate CL. Perhaps if someone has ideas for precisely how to do it and knows more about the setup you could lead the way by refactoring one of the other sanitizers -- I'm not sure what the COMPILER_RT_STANDALONE_BUILD supports (comments in the code make it sound unsupported; does it support building with an installed clang rather than a local build dir and in that case won't llvm-config not work unless we install our header which seems overkill; if it really requires a local llvm build dir in what sense is it standalone; etc.)

bruening marked 3 inline comments as done.May 23 2016, 1:11 PM
bruening added inline comments.
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
70 ↗(On Diff #57976)

These do not change: our general shadow mapping handles all scales we will want.

309 ↗(On Diff #57976)

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.

409 ↗(On Diff #57976)

See NumFastpaths.

424 ↗(On Diff #57976)

We are explicitly fine with races accessing our shadow memory. This is documented in the shadow interface. I will add another comment here.

433 ↗(On Diff #57976)

Ditto.

bruening marked an inline comment as done.May 23 2016, 1:29 PM
bruening updated this revision to Diff 58147.May 23 2016, 1:30 PM

Address reviewer comments.

aizatsky added inline comments.May 24 2016, 2:13 PM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
412 ↗(On Diff #58147)

I'm sorry but I (and people in my cube) don't really understand what "straddle a cache line" mean. Could you rewrite the comment to explain the reasoning better? What is the plan for TypeSize != 8? Slow path?

414 ↗(On Diff #58147)

NumFastpaths is not incremented though.

bruening marked an inline comment as done.May 24 2016, 3:04 PM
bruening added inline comments.
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
414 ↗(On Diff #58147)

It is incremented by the caller based on the return value. The number of slowpaths is also available by computing (NumInstrumentedStores + NumInstrumentedLoads) - NumFastpaths.

bruening updated this revision to Diff 58327.May 24 2016, 3:06 PM

Clarifies cache line check comment.

aizatsky added inline comments.May 24 2016, 3:20 PM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
419 ↗(On Diff #58327)

This comment doesn't seem to correspond to the code.
Isn't it "Bail to the slowpath for unaligned access"?

For multiple cache lines, don't you want to compare TypeSize with cache line size?

425 ↗(On Diff #58327)

This sentence is immediately contradicted by the next. "each cache line" vs "we've already ruled out".

bruening added inline comments.May 24 2016, 3:28 PM
lib/Transforms/Instrumentation/EfficiencySanitizer.cpp
419 ↗(On Diff #58327)

A memory access that is aligned to its size is guaranteed to only touch one cache line. (So long as it's <= 64 bytes which seems a safe assumption as memcpy, etc. intrinsics are handled elsewhere: this is a single load or store.)

425 ↗(On Diff #58327)

The first sentence is talking about the tool's instrumentation in general. I will update it to remove any confusion.

bruening updated this revision to Diff 58339.May 24 2016, 3:34 PM

Expand comments around single vs multiple cache lines.

aizatsky accepted this revision.May 24 2016, 3:38 PM
aizatsky edited edge metadata.
This revision is now accepted and ready to land.May 24 2016, 3:38 PM
This revision was automatically updated to reflect the committed changes.