Tapir interaction with Global Value Numbering (GVN)

[ehein6]

I'm implementing Tapir lowering for a new backend. Found a case where the Global Value Numbering (GVN) optimization breaks my code by hoisting a load above a lowered call to cilk_sync. I see there is Tapir-specific code in the GVN pass to block this optimization when a load depends on a detach. For example, this block of code activates when GVN runs before Tapir lowering:

Tapir-LLVM/lib/Transforms/Scalar/GVN.cpp

Lines 1335 to 1347 in 9fa482d

	// If we depend on a detach instruction, reject.
	for (unsigned i = 0, e = NumDeps; i != e; ++i) {
	MemDepResult DepInfo = Deps[i].getResult();
	if (!(DepInfo.getInst()))
	continue;
	if (isa<DetachInst>(DepInfo.getInst())||
	isa<SyncInst>(DepInfo.getInst())) {
	DEBUG(dbgs() << "GVN: Cannot process" << *LI <<
	" due to dependency on" <<
	*(DepInfo.getInst()) << "\n");
	return false;
	}
	}

In my case, GVN runs again after lowering, and by this point there's nothing special about the sync or detach blocks. What constraint is supposed to prevent this? Or is it just not safe to do GVN after lowering?

Probably you would like to see an example, will post later.

[VictorYing]

Is your lowered call to cilk_sync treated by LLVM as readonly or readnone? I don't have access to that emusolutions repo you linked to, but, can you make the construct you're lowering your sync to into something that LLVM thinks may write to memory that might alias with the load? This approach (making syncs (and spawns) appear to have side effects in memory to prevent reordering) is discussed in the alias analysis section of the original Tapir paper. A quick hack to try might be to just insert a memory barrier/fence next to the sync during lowering.

[ehein6]

I see now that the __cilk_sync for x86, once inlined, emits volatile loads and stores, in addition to inline assembly. Probably this is what blocks GVN. Thanks for the tip, I'll give this a try.

I don't have access to that emusolutions repo you linked to

Sorry about that, edited my post to point to the public fork

[ehein6]

Converting the loads within __cilk_sync to volatile solved the problem, thanks.