Page MenuHomePhabricator

[IR] Begin removal of TerminatorInst by removing successor manipulation.
ClosedPublic

Authored by chandlerc on May 29 2018, 3:29 AM.

Details

Summary

The core get and set routines move to the Instruction class. These
routines are only valid to call on instructions which are terminators.

The iterator and *generic* range based access move to CFG.h where all
the other generic successor and predecessor access lives. While moving
the iterator here, simplify it using the iterator utilities LLVM
provides and updates coding style as much as reasonable. The APIs remain
pointer-heavy when they could better use references, and retain the odd
behavior of operator* and operator-> that is common in LLVM
iterators. Adjusting this API, if desired, should be a follow-up step.

Non-generic range iteration is added for the two instructions where
there is an especially easy mechanism and where there was code
attempting to use the range accessor from a specific subclass:
indirectbr and br. In both cases, the successors are contiguous
operands and can be easily iterated via the operand list.

This is the first major patch in removing the TerminatorInst type from
the IR's instruction type hierarchy. The steps that I have planned out
(although there are likely to be some minor ones) look like the
following:

  • Remove the few remaining APIs of TerminatorInst, replacing them with existing predicates in Instruction or adding new ones if needed.
  • Replacing isa<TerminatorInst> and dyn_cast<TerminatorInst> with appropriate uses of isTerminator.
  • Replacing TerminatorInst in parameter APIs with just Instruction and potentially an assert on isTerminator.
  • Replacing TerminatorInst in the return of the remaining APIs (not updated above) with Instruction. Most notably, the getTerminator method.
  • Removing TerminotarInst type completely.
  • Making InvokeInst and CallInst derive from a common base class w/ most of the logic currently in CallInst.
  • Replacing uses of CallSite everywhere with the common base class.

These are likely to be quite mechanical, and I suspect are probably best done
with post-commit review. Please lete me know if there are specific
patches in this series you'd like to do pre-commit review on and I'll
happily break them out. Naturally, if any end up with significant
questions in them, I'll post a review for that.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.May 29 2018, 3:29 AM
dberris added inline comments.
llvm/include/llvm/IR/CFG.h
193 ↗(On Diff #148871)

Maybe this should be a non-member friend, and works with const and non-const versions of compatible type?

friend inline bool operator==(const Self& L, const Self& R) {
  return L.Idx == R.Idx;
}
chandlerc added inline comments.May 29 2018, 9:52 AM
llvm/include/llvm/IR/CFG.h
193 ↗(On Diff #148871)

It has to be a member for the underlying iterator helper to work (due to how that underlying helper is implemented).

Is there really a problem to solve here though? Not sure what the benefit is of this in the absence of a type hierarchy... We don't do it very consistently in LLVM anywhere.

rnk added inline comments.May 29 2018, 2:16 PM
llvm/include/llvm/IR/Instructions.h
3263 ↗(On Diff #148871)

What are these used for? It doesn't seem like they are called in any way in this code. Do these keep existing calls to Br->successors() compiling?

dberris added inline comments.May 29 2018, 5:22 PM
llvm/include/llvm/IR/CFG.h
193 ↗(On Diff #148871)

Ah, that's fair -- didn't know that the facade requires equality to be defined as a member.

The only problems the friend declaration solves are the overload resolution issue (if you want to have type deduction of the RHS to support slightly different types) and forces ADL when defined inline -- i.e. the only way to find the operator is through ADL.

Consider:

template <class T>
class C {
  template <class U> friend bool operator==(const C& L, const C<U>& R) {
    return ...;
  }
  ...
};

This allows C<int> and C<const int> types to be comparable, without having to make any special considerations. It also only allows for the equality comparison to be found with ADL, and no other way (AFAICT).

Whether that needs to be more widely adopted in LLVM/Clang is a separate issue, probably not worth spending a lot more time on.

chandlerc marked 3 inline comments as done.May 29 2018, 5:27 PM
chandlerc added inline comments.
llvm/include/llvm/IR/CFG.h
193 ↗(On Diff #148871)

Yeah, I think this is going to be fine for our somewhat limited needs for now. Thanks for the feedback though!

llvm/include/llvm/IR/Instructions.h
3263 ↗(On Diff #148871)

Correct. THere were a few places that were calling it directly on a BranchInst and it seemed easy enough to implement that in a more obvious fashion than going through the (quite high cost) generic successors logic.

chandlerc marked an inline comment as done.

Adding James as a reviewer since I know he's interested in this stuff...

hfinkel accepted this revision.Jun 5 2018, 2:47 AM
hfinkel added a subscriber: hfinkel.

LGTM

These are likely to be quite mechanical, and I suspect are probably best done with post-commit review.

I agree. The others sound sufficiently mechanical for post-commit review.

llvm/include/llvm/IR/CFG.h
195 ↗(On Diff #148871)

Should BasicBlock here be BlockT?

198 ↗(On Diff #148871)

Same here?

This revision is now accepted and ready to land.Jun 5 2018, 2:47 AM
chandlerc marked 5 inline comments as done.Jul 10 2018, 2:08 AM

Any last comments here? Hal has given LGTM, and so I'm planning to move forward with this tomorrow.

(I've rebased it and addressed the two minor comments from Hal...)

Hi Chandler, are you still planning to work on that?

Thanks, Alexander

Hi Chandler, are you still planning to work on that?

Thanks, Alexander

Yes, really sorry for the delay here.

This has been held up by finishing off security related work, but should be moving again once the next patch there is finished.

Hi Chandler, are you still planning to work on that?

Thanks, Alexander

Yes, really sorry for the delay here.

This has been held up by finishing off security related work, but should be moving again once the next patch there is finished.

FYI, I've rebased and will be landing this and subsequent patches now.

This revision was automatically updated to reflect the committed changes.