cmd/compile: move instrumention into an ssa pass?

[josharian]

This is a naive question intended to open discussion.

race/msan instrumentation currently happens between walk and SSA generation. As a result, as we slowly migrate walk rewrites to SSA generation and/or SSA itself, racewalk will slowly fall out of sync and/or be a source of added work. And the simpler ontology of SSA might prove helpful in increasing instrumentation coverage.

Does it make sense to consider moving racewalk to a generic SSA pass? If so, opinions/considerations about implementation details?

I am not volunteering at the moment to do this work, although I did bump into this question while satisfying my curiosity about what it would be like to handle ORANGE directly during SSA generation. (Answer: It's a fair amount of work.)

cc @dvyukov @ianlancetaylor @randall77

[dvyukov]

Yes, it makes sense.

It should make the pass considerably simpler and hopefully eliminate all the corner cases that we still continue to hit episodically.

It may also help to implement some of the goodness we have in llvm. IIRC that PointerMayBeCaptured eliminated ~1/3 of instrumentation. Also deduplication at least within the same basic block:

void ThreadSanitizer::chooseInstructionsToInstrument(
    SmallVectorImpl<Instruction *> &Local, SmallVectorImpl<Instruction *> &All,
    const DataLayout &DL) {
  SmallSet<Value*, 8> WriteTargets;
  // Iterate from the end.
  for (Instruction *I : reverse(Local)) {
    if (StoreInst *Store = dyn_cast<StoreInst>(I)) {
      Value *Addr = Store->getPointerOperand();
      if (!shouldInstrumentReadWriteFromAddress(Addr))
        continue;
      WriteTargets.insert(Addr);
    } else {
      LoadInst *Load = cast<LoadInst>(I);
      Value *Addr = Load->getPointerOperand();
      if (!shouldInstrumentReadWriteFromAddress(Addr))
        continue;
      if (WriteTargets.count(Addr)) {
        // We will write to this temp, so no reason to analyze the read.
        NumOmittedReadsBeforeWrite++;
        continue;
      }
      if (addrPointsToConstantData(Addr)) {
        // Addr points to some constant data -- it can not race with any writes.
        continue;
      }
    }
    Value *Addr = isa<StoreInst>(*I)
        ? cast<StoreInst>(I)->getPointerOperand()
        : cast<LoadInst>(I)->getPointerOperand();
    if (isa<AllocaInst>(GetUnderlyingObject(Addr, DL)) &&
        !PointerMayBeCaptured(Addr, true, true)) {
      // The variable is addressable but not captured, so it cannot be
      // referenced from a different thread and participate in a data race
      // (see llvm/Analysis/CaptureTracking.h for details).
      NumOmittedNonCaptured++;
      continue;
    }
    All.push_back(I);
  }
  Local.clear();
}

[mdempsky]

Incidentally, I was looking at this some last week.

The big issue I had was whether to implement instrumentation as an SSA pass or during buildssa. Normally loads don't modify the memory variable, but when instrumented with a sanitizer function call they do. If we were to handle instrumentation as a pure SSA pass, it might need to rewrite memory variable references and insert phi nodes.

I did have a semi-working CL that emitted instrumentation calls during buildssa (basically just emitted a call before each OpLoad/OpStore/OpMove/OpZero), but it produced only about 80-90% as many calls. I haven't yet investigated why.

[dvyukov]

Normally loads don't modify the memory variable, but when instrumented with a sanitizer function call they do.

Implementing it like a normal pass sounds better long term. What's the problem with loads?

but it produced only about 80-90% as many calls

It may be good. First thing is to just run all race test and go test -race std. If that works, we just need to look at some cases where ssa pass does not insert instrumentation.

[josharian]

If reasonable, it'd be nice to do it as an SSA pass rather than during buildssa, for encapsulation and understandability reasons--buildssa is already unwieldy, and it will only grow from here.

Normally loads don't modify the memory variable, but when instrumented with a sanitizer function call they do.

It might take some work to make the scheduler happy with this, but could we simply leave sanitizer function calls as non-memory-modifying?

[mdempsky]

What's the problem with loads?

Basically the signatures of SSA Load and Store operations are:

Load :: (*T, Memory) -> T
Store :: (*T, T, Memory) -> Memory

SSA also requires that Memory objects be linearly threaded through the computation. (Similar to monads in Haskell.)

So Stores are necessarily linearized, and Loads are freely rearranged between two Stores. Also, Calls behave a lot like Stores:

Call :: (Function, Memory) -> Memory

Instrumenting a Store is easy, because it amounts to just rewriting:

mem2 = Store(ptr, val, mem1)

into:

memtmp = Call(racewrite, ptr)
mem2 = Store(ptr, val, memtmp)

Since Store already produced a new Memory value, introducing a new Memory temporary for the instrumentation call is a purely local rewrite.

Instrumenting Load is trickier though, because if we have something like:

x := 0
if y != nil {
    x = *y
}

that is currently something like:

x := Const(0)
if NEQ(y, Const(nil)) {
    x = Load(y, mem0)
}

But as an SSA pass, we would have to rewrite it to:

x := Const(0)
if NEQ(y, Const(nil)) {
    mem1 = Call(raceread, y, mem0)
    x = Load(y, mem1)
}
mem2 = Phi(mem0, mem1)
// subsequent references to mem0 have to also be updated to mem2

[randall77]

I support moving race instrumentation to an SSA pass.

I was thinking about introducing a non-memory call for side-effect free calls. This would be generically useful for a bunch of runtime support calls, like complex division. We could use a similar call for instrumenting loads.
I've been waiting to pursue this idea until calling convention changes go in. Part of the reason calls take a memory argument is to make sure argument marshaling happens before the call does. That gets a lot easier with a register-based convention.
Of course, these are all special runtime calls, so we could special-case their calling conventions on an as-needed basis.

[mdempsky]

Naively (i.e., I still largely treat package ssa as a black box), being able to express side-effect free calls SGTM.

Until we have that though, is there any opposition to moving racewalk into buildssa? My WIP CL was relatively non-intrusive after refactoring out the OpLoad/OpStore construction patterns, and I think could still be migrated to a proper SSA pass later.

[josharian]

Of course, these are all special runtime calls, so we could special-case their calling conventions on an as-needed basis.

I can't decide whether this is exciting or horrifying. It might be helpful for race calls, so that they can check args/results without disrupting the original call.

Until we have that though, is there any opposition to moving racewalk into buildssa?

Fine by me.

[randall77]

I guess I'm not really for moving the code twice if we can avoid it.
Do the race calls return anything? If not, we can just use a special call that takes a memory arg but returns void. That could be scheduled anywhere the input memory state is live. That's an easy change to the ssa backend.

[mdempsky]

Do the race calls return anything?

No. They just take an address and sometimes a size as input. No outputs.

[dvyukov]

Yes, not outputs, and the calls don't touch anything that can possibly concern the compiler. The only case when the calls callback into Go world is symbolization callback during race reporting, but it runs non-instrumented runtime code.
The calls can be moved if they don't cross any synchronization operations, but it is probably not very important.