Broken/dead code for finding reattach points for moving static allocas in extractDetachBodyToFunction()

[VictorYing]

In extractDetachBodyToFunction(), early on, the reattaches for the current detach are changed to branches: https://github.com/wsmoses/Tapir-LLVM/blob/1482504e234a65bffc8c54de8de9fc877822345d/lib/Transforms/Tapir/TapirUtils.cpp#L258-L263 Later in the function, we search for actual ReattachInsts for determining the exit points of the task needed for a call to MoveStaticAllocasInBlock(): https://github.com/wsmoses/Tapir-LLVM/blob/1482504e234a65bffc8c54de8de9fc877822345d/lib/Transforms/Tapir/TapirUtils.cpp#L352-L366 But since the reattaches have already been replaced with branches, it seems to me that ReattachPoints will always be empty. Surely this is not what is intended? It seems there aren't any tests that verify whether this call to MoveStaticAllocasInBlock() actually performs all its intended effects.

[VictorYing]

The extra funny thing is, if we wanted access to a list of reattaching blocks, there already was one computed earlier in the function that could be reused: https://github.com/wsmoses/Tapir-LLVM/blob/1482504e234a65bffc8c54de8de9fc877822345d/lib/Transforms/Tapir/TapirUtils.cpp#L253-L255

[neboat]

At a guess, this looks like an artifact of cherry-picking some commit from another branch. But I would need to look deeper to figure out what happened.

[VictorYing]

If I'm reading the code right, this appears to have been a problem since 285ff4617892da4132f4a0aded992dcc4c5af6d5, which is the commit that introduced MoveStaticAllocasInClonedBlock(). In that commit, in extractDetachBodyToFunction(), we had the reattaches being replaced with branches via this call to populateDetachedCFG(): https://github.com/wsmoses/Tapir-LLVM/blob/285ff4617892da4132f4a0aded992dcc4c5af6d5/lib/Transforms/Tapir/CilkABI.cpp#L1001 followed by code introduced in that commit which attempts to collect no-longer existing reattaches: https://github.com/wsmoses/Tapir-LLVM/commit/285ff4617892da4132f4a0aded992dcc4c5af6d5#diff-c28f34ee61c1819df521c7ba06215730R1085 https://github.com/wsmoses/Tapir-LLVM/blob/285ff4617892da4132f4a0aded992dcc4c5af6d5/lib/Transforms/Tapir/CilkABI.cpp#L1085-L1098

That commit actually introduced calls to MoveStaticAllocasInClonedBlock() in two places, the one in LoopSpawning looks like it works correctly, and it looks like that same code was copied and pasted into extractDetachBodyToFunction() without testing.

Overall, I don't know enough about what the rules of where allocas go and how they interact with Tapir to understand whether any of this is important.