Page MenuHomePhabricator

[IRSim][IROutliner] Limit to extracting regions that only require inputs
ClosedPublic

Authored by AndrewLitteken on Sep 1 2020, 1:29 PM.

Details

Summary

Extracted regions can have both inputs and outputs. In addition, the CodeExtractor removes inputs that are only used in llvm.assumes, and sunken allocas (values are used entirely in the extracted region as denoted by lifetime intrinsics). We also cannot combine sections that have different constants in the same structural location, and these constants will have to be elevated to arguments. This patch limits the extracted regions to those that only require inputs, and do not have any other special cases.

We test that we do not outline the wrong constants in:

  • test/Transforms/IROutliner/outliner-different-constants.ll
  • test/Transforms/IROutliner/outliner-different-globals.ll
  • test/Transforms/IROutliner/outliner-constant-vs-registers.ll

We test that correctly outline in:

  • test/Transforms/IROutliner/outlining-same-globals.ll
  • test/Transforms/IROutliner/outlining-same-constants.ll
  • test/Transforms/IROutliner/outlining-different-structure.ll

Diff Detail

Event Timeline

AndrewLitteken created this revision.Sep 1 2020, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 1:29 PM
AndrewLitteken added a reviewer: paquette.

Updating for clang-format.

Updating to reflect changes in other patches.

jroelofs added inline comments.
llvm/lib/Transforms/IPO/IROutliner.cpp
204–206

It's ok to lean on Optional's internal assertions.

220

Optional<bool>'s can be pretty confusing. Probably better to make constantMatches take a Constant* and return a regular bool, and then have this be:

if (Constant *C = dyn_cast<Constant>(V)) {
  if (!constantMatches(C, GVN, GVNToConstant))
    return false;
  continue;
}
489

Is there a performance issue with declaring NotSame here? Sinking declarations helps show the reader that there are no loop-carried dependencies.

jroelofs added inline comments.Sep 18 2020, 10:23 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
517

Referencing the moved-from object, though legal, seems strange. Maybe the two loops here ought to process the regions into differently named worklists. Since the first loop hasn't outlined them yet, maybe call that one OutlineableRegions and the second OutlinedRegions?

paquette added inline comments.Sep 18 2020, 10:39 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
489

I think this is a leftover from this: https://llvm.org/docs/ProgrammersManual.html#vector

But I'm not sure if this applies to DenseSet.

plotfi added inline comments.Sep 18 2020, 10:50 AM
llvm/include/llvm/Transforms/IPO/IROutliner.h
192

Can these white space changes be in an NFC commit?

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

can't this just be

return (Inserted || (GVNToConstantIt->second == CST));

Avoid the branch and confusing code here.

266

Does this need to be a SetVector of Value pointers? Can it be references instead? You can drop the assert on nullptr then.

paquette added inline comments.Sep 18 2020, 11:02 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
342

This doesn't do anything

paquette added inline comments.Sep 18 2020, 11:06 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
335

Does IgnoreRegion have to be a member variable at all?

Maybe make this and getCodeExtractorArguments return a bool?

e.g.

return getCodeExtractorArguments(Region, Inputs, ArgInputs);

Then the caller of this function can just check whether or not this function succeeded via the bool?

paquette added inline comments.Sep 18 2020, 11:11 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
508

If findAddInputsOutputs returned a bool, you could just do

if (findAddInputsOutputs(M, *OS) {
  // Found the outputs and inputs. We can outline this.
  OutlinedRegions.push_back(OS);
  continue;
}

// Couldn't find the outputs/inputs. Bail out.
OS->reattachCandidate();
paquette added inline comments.Sep 18 2020, 11:17 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
245

Instead of having IgnoreGroup as a member variable, would it make sense to have this return a bool?

Then this entire function could probably just be one all_of.

DenseMap<unsigned, Constant *> Dummy;
return all_of(Regions, [&Dummy, &NotSame](OutlinableRegion *Region){return collectRegionsConstants(...);});
plotfi added inline comments.Sep 18 2020, 11:28 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
316–330

Why cant these conditional blocks be merged? ie:

if (Outputs.size() && ArgInputs.size() != OverallInputs.size()) {
  Region.IgnoreRegion = true;
  return;
}
508

Can you add a comment on why IgnoreRegion dictates if push_back or reattachCandidate should be called here?

jroelofs added inline comments.Sep 18 2020, 3:48 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
302

I'd sink this to right before where it's used.

llvm/include/llvm/Transforms/IPO/IROutliner.h
192

I can just make them correct in the commit that introduces this section.

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

Yes, this is from the CodeExtractor, so unless I reconstruct the SetVector, it has to stay as pointers :/.

316–330

They can be, they were just simplified for readability so I could included more detailed comments for why we were skipping these cases.

335

That would probably work, and be a bit cleaner.

517

That's a better idea. And it will prevent a second move later on as well.

Updating based on comments.

paquette accepted this revision.Nov 9 2020, 9:12 AM

LGTM with nits on the comments.

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

Update param name in comments?

187

Parameter is not documented?

267
This revision is now accepted and ready to land.Nov 9 2020, 9:12 AM