Page MenuHomePhabricator

[IROutliner] Outline across branches with multiple exits
ClosedPublic

Authored by AndrewLitteken on Jul 28 2021, 1:04 PM.

Details

Summary

When we start outlining across branches, there is the possibility that we will have two different blocks with different output locations, or a single branch that goes to two blocks outside of the region that is being outlined. While the CodeExtractor provides most of the mechanisms by using the return value of the extracted function as the input to a switch statement to correctly branch to the correct location, we need special handling for different output schemas to each location.

This is done by repeating the existing storing scheme for each different exit block. We have a map from the return values used, to the basic block that is used to store the outputs for that particular exit block within the outlined function. Then if needed, we create a switch statement for each return block to branch to the correct set of stored outputs.

Diff Detail

Event Timeline

AndrewLitteken created this revision.Jul 28 2021, 1:04 PM
AndrewLitteken requested review of this revision.Jul 28 2021, 1:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2021, 1:04 PM

Updating tests, fixing bugs and making the code cleaner.

paquette added inline comments.Tue, Aug 31, 3:18 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
165

probably don't need variables here?

445
457

the if + else if here set the same value; merge them into one if?

524

comment?

528–533

comment?

530

Might as well drop the variable here, since it's only used once.

1224–1225

Do you actually need two variables here? Why not just one called "Mismatch" or something?

1224–1225
1379

It looks like there's some similar code to this later down the line. Can you refactor this a bit?

1610

This pattern (creating a BB with a name + indices then inserting into a list) appears a lot. Would it make sense to have a helper function for this?

  • Updating for style
  • Refactoring BasicBlock creation and insertion based on an old list
  • Refactoring looking for empty blocks
paquette added inline comments.Thu, Sep 2, 2:26 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
153

might as well auto here...

156

I don't think this check is necessary?

456

why 16?

1128

can you just use dyn_cast versus isa + cast?

AndrewLitteken added inline comments.Thu, Sep 2, 2:54 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
456

It's what the code extractor uses. IT creates functions of type void, i1 or i16

Updating types, removing unnecessary checks

paquette accepted this revision.Tue, Sep 7, 2:50 PM

I think this looks fine with a nit on a comment.

llvm/lib/Transforms/IPO/IROutliner.cpp
146

So this isn't really if it has constants, right? This actually expects all constants. Maybe this comment should reflect that?

This revision is now accepted and ready to land.Tue, Sep 7, 2:50 PM
This revision was landed with ongoing or failed builds.Wed, Sep 8, 9:08 AM
This revision was automatically updated to reflect the committed changes.