Page MenuHomePhabricator

[IRSim][IROutliner] Adding support for consolidating functions with different output arguments.
AcceptedPublic

Authored by AndrewLitteken on Sep 8 2020, 10:08 AM.

Details

Reviewers
paquette
jroelofs
Summary

Certain regions can have values introduced inside the region that are used outside of the region. These may not be the same for each similar region, so we must create one over arching set of arguments for the consolidated function.

We do this by iterating over the outputs for each extracted function, and creating as many different arguments to encapsulate the different outputs sets. For each output set, we create a different block with the necessary stores from the value to the output register. There is then one switch statement, controlled by an argument to the function, to differentiate which block to use.

Changed Tests for consistency:

  • llvm/test/Transforms/IROutliner/extraction.ll
  • llvm/test/Transforms/IROutliner/illegal-assumes.ll
  • llvm/test/Transforms/IROutliner/illegal-memcpy.ll
  • llvm/test/Transforms/IROutliner/illegal-memmove.ll
  • llvm/test/Transforms/IROutliner/illegal-vaarg.ll

Tests to test new functionality:

  • llvm/test/Transforms/IROutliner/outlining-different-output-blocks.ll
  • llvm/test/Transforms/IROutliner/outlining-remapped-outputs.ll
  • llvm/test/Transforms/IROutliner/outlining-same-output-blocks.ll

Diff Detail

Event Timeline

AndrewLitteken created this revision.Sep 8 2020, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2020, 10:08 AM

Removing uses of std::set.

jroelofs added inline comments.
llvm/lib/Transforms/IPO/IROutliner.cpp
340

const DenseMap &?

364

const DenseMap &?

394–397

extracted

518

This can also be ArrayRefd. See SetVector::getArrayRef()

791

Should this be an ArrayRef, since the vector contents aren't modified by this function?

840

const DenseMap &?

854

Since you have to fix up for the lifetime intrinsics anyway and there's no instructionsWithoutDebugOrLifetime(), maybe it makes more sense to just iterate over everything, and add the if (isa<DebugInfoIntrinsic>(Inst)) continue; check.

872

... which makes this part less awkward.

894

Likewise, I think this can also be ArrayRef-ified.

969–970

I'd sink this into the loop to make it clear you don't have a loop-carried dependence on it.

1052

Outputs should probably be const, or made into an ArrayRef, since it's not modified here.

Updating for comments. Also removed function from this diff to the next since it was not used here.

jroelofs added inline comments.Sep 15 2020, 10:44 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
364

I think ArgInputs can also be const.

738

The dyn_cast<> followed by dereferencing w/o checking smells fishy. This should either be a cast<>, or have an explicit test.

818

What if FIt has debug/lifetime instructions that BB doesn't? The dual iteration thing going on here seems... iffy.

842–847
975

I think this is a good place to use a Twine, which avoids some unnecessary allocation.

Rejiggering the rickety double-iteration to collect the instructions separately.

jroelofs accepted this revision.Sep 18 2020, 9:22 AM

LGTM

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

Much better, thanks!

This revision is now accepted and ready to land.Sep 18 2020, 9:22 AM

Updating based on previous changes.