This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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
355

const DenseMap &?

379

const DenseMap &?

407–412

extracted

540

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

814

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

863

const DenseMap &?

877

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.

895

... which makes this part less awkward.

917

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

995–996

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

1078

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
379

I think ArgInputs can also be const.

761

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

841

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

865–870
1001

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
841

Much better, thanks!

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

Updating based on previous changes.