Page MenuHomePhabricator

Syndicate duplicate code between CallInst and InvokeInst
ClosedPublic

Authored by serge-sans-paille on Dec 1 2017, 7:19 AM.

Details

Summary

That's a follow-up to a discussion we had we @chandlerc on Callinst and InvokeInst. Still not in a final stage but published here to gather advices.

Possible follow-up if we add a virtual non-template base class to the CallableInst, then with the proper isa overload we may be able to remove the need of CallSite and ImmutableCallSite

Diff Detail

Repository
rL LLVM

Event Timeline

serge-sans-paille edited the summary of this revision. (Show Details)

No longer using CRTP to avoid extra cost. I could not find a way to create a separate type hierarchy because of the heavy dependance on User. Instead I cunningly hooked the heritance tree, resulting in most of the code going in a common parent class.

This review is purely code cleaning, it avoids a lot of redundancy between CallInst and InvokeInst. Reviews are welcome!

vsk added a subscriber: vsk.Jan 3 2018, 3:07 PM

This looks like a nice cleanup.

Do you think it's worth adding an implementation of classof() etc. to CallableBase which calls llvm_unreachable?

include/llvm/IR/Instructions.h
1358 ↗(On Diff #125689)

nit: ", actually" -> "(InvokeInst and CallInst)."

@vsk: added classof as unreachable, fixing comment and improved CallableBase mechanic
@chandlerc your turn :-)

Adding @sanjoy to the revies, based on `svn praise`.

Up! What a great and entertaining patch to review :-)

arnt added a subscriber: arnt.Feb 15 2018, 3:27 AM

Looks like a clear improvement, but one minor point: "Callable" sounds like it's a template/base for Function and perhaps things that implement intrinsics, not like a base for instructions that call. CallerBase perhaps?

Class names updated, thanks @arnt for the review!

I think I gathered enough feedback, I'll merge that tomorrow.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2018, 5:33 AM
This revision was automatically updated to reflect the committed changes.