Page MenuHomePhabricator

[CodeExtractor] Ensuring ordering of exitStub creation
ClosedPublic

Authored by AndrewLitteken on Aug 24 2021, 12:59 PM.

Details

Summary

Previously the CodeExtractor created exit stubs, and the subsequent return value of the outlined function based on the order of out-of-region blocks after splitting any phi nodes, and collecting the blocks to be outlined. This could cause differences in order if there was a difference of exit block phi nodes between the two regions. This patch moves the collection of the output target blocks to be before this occurs, so that the assignment of target block to output value will be the same, regardless of the contents of the output block.

Diff Detail

Event Timeline

AndrewLitteken requested review of this revision.Aug 24 2021, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2021, 12:59 PM
jroelofs added inline comments.Aug 24 2021, 1:27 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1268

Should there be an assert:

assert(NumExitBlocks < 0xffff && "too many exit blocks for switch");

or do you think that's too unlikely to matter?

1283

Doesn't the switch need the same type as the cases, or am I misunderstanding what's going on here?

1294

Code following this doesn't assign into the variable, so we may as well drop the & on this one for clarity.

AndrewLitteken added inline comments.Aug 24 2021, 2:00 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1268

I think it's pretty unlikely that there will ever by that many exits out of a function.

1283

They do, but if there are only two exit blocks, there are only two options, a 0 and 1, returned out of the outlined function. If that's the case, the CodeExtractor simplifies this to a branch instruction anyway.

jroelofs added inline comments.Aug 24 2021, 2:01 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1268

👍

paquette added inline comments.Aug 25 2021, 1:32 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1268

I think it would be good to assert anyway if it's possible that it will happen. In the event that it does, I'd prefer to debug an assert than a later crash.

Updating with assert.

paquette added inline comments.Aug 31 2021, 2:53 PM
llvm/lib/Transforms/Utils/CodeExtractor.cpp
1291

Reduce indentation?

 if (Blocks.count(TI->getSuccessor(i)))
   continue;
...
1651

reduce indentation here too?

This revision is now accepted and ready to land.Sep 8 2021, 10:24 AM
This revision was landed with ongoing or failed builds.Sep 8 2021, 3:15 PM
This revision was automatically updated to reflect the committed changes.