Page MenuHomePhabricator

[CodeExtractor] Do not extract unsafe lifetime markers

Authored by vsk on Dec 20 2018, 3:44 PM.



Lifetime markers which reference inputs to the extraction region are not
safe to extract. Example ('rhs' will be extracted):

  	      x = alloca
  	      y = alloca
         /              \
       lhs:             rhs:
lifetime_start(x)   lifetime_start(x)
use(x)    	      lifetime_start(y)
lifetime_end(x)     use(x, y)
lifetime_start(y)   lifetime_end(y)
use(y)              lifetime_end(x)
lifetime_end(y)     ...

Prior to extraction, the stack coloring pass sees that the slots for 'x'
and 'y' are in-use at the same time. After extraction, the coloring pass
infers that 'x' and 'y' are *not* in-use concurrently, because markers
from 'rhs' are no longer available to help decide otherwise.

This leads to a miscompile, because the stack slots actually are in-use
concurrently in the extracted function.

Fix this by moving lifetime start/end markers for memory regions defined
in the calling function around the call to the extracted function.

Fixes (rdar://45939472).

Diff Detail


Event Timeline

vsk created this revision.Dec 20 2018, 3:44 PM
davidxl added inline comments.Dec 20 2018, 4:18 PM
12 ↗(On Diff #179182)

The purpose of the extraction is to shrink the alloca call into the outlined function and thus reducing call overhead of the outline function. This fix breaks that.

vsk added inline comments.Dec 20 2018, 5:44 PM
12 ↗(On Diff #179182)

Here, the alloca remains in 'caller' both without and with this patch. Are you suggesting that lifetime markers have an affect on codegen even when the referenced alloca is in a different function? I don't see why this would be, but if so, would the right fix be to only move lifetime markers out of outlined code when there are conflicting markers in the caller?

davidxl added inline comments.Dec 21 2018, 11:13 AM
12 ↗(On Diff #179182)

Sorry I mis-read the test case.

davidxl added inline comments.Dec 21 2018, 11:28 AM
1189 ↗(On Diff #179182)

document the second parameter?

1199 ↗(On Diff #179182)

Is there a common utility function to detect lifetime marker intrinsics?

1200 ↗(On Diff #179182)

Is there a better API to access the mem operand instead of using hard coded operand number?

I'm concerned about skipping processing markers for outputs. Currently HotColdSplit and partial inlining do not outline function if number of outputs is not zero (by calling findInputsOutputs()), but this check is unreliable because there is some processing of region before extracting (splitReturnBlocks, splitPHINodesOfEntry, splitPHINodesOfExit) and input/output parameters can be changed (their number may become non-zero). So another question is why this check should be done at all (does it bypass some known problem?) Seems that CodeExtractor does not impose such restrictions.

vsk planned changes to this revision.Dec 21 2018, 11:57 AM

(Marking this WIP while I address @davidxl 's feedback.)

@kachkov98 -- The |outputs| > 0 case had known issues, but I believe they were all resolved by D55018. I have a follow-up planned to enable hot/cold splitting when |outputs| > 0.

Secondly, I think you're correct, it does seem beneficial to add lifetime markers around the reloads for output values. However, I don't think it's needed for correctness, because it's impossible for there to be lifetime markers for output allocas in the caller function prior to extraction. For that reason I'd prefer to do this as a follow-up.

vsk updated this revision to Diff 179333.Dec 21 2018, 12:34 PM
vsk marked 4 inline comments as done.
1199 ↗(On Diff #179182)

I'll rebase this on top of D56019.

1200 ↗(On Diff #179182)

Unfortunately not. I'll leave a better comment to explain what is happening here.

This revision is now accepted and ready to land.Jan 4 2019, 9:15 AM
davidxl accepted this revision.Jan 4 2019, 9:30 AM


This revision was automatically updated to reflect the committed changes.