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

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
1357

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.