Page MenuHomePhabricator

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

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

Details

Reviewers
paquette
plotfi
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
202–204

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

218

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;
}
466

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
495

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
466

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
190

Can these white space changes be in an NFC commit?

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

can't this just be

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

Avoid the branch and confusing code here.

264

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
340

This doesn't do anything

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

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
483

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
243

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
314–328

Why cant these conditional blocks be merged? ie:

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

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
300

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

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

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

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

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

314–328

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

333

That would probably work, and be a bit cleaner.

495

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

Updating based on comments.