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
354

const DenseMap &?

378

const DenseMap &?

405–410

extracted

539

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

813

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

862

const DenseMap &?

876

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.

894

... which makes this part less awkward.

916

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

994–995

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

1077

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
378

I think ArgInputs can also be const.

760

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

840

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

864–869
1000

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
840

Much better, thanks!

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

Updating based on previous changes.