[PATCH] D54788: [TI removal] Leverage the fact that TerminatorInst is gone to create a normal base class that provides all common "call" functionality.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc created this revision.
chandlerc added a reviewer: bkramer.
Herald added subscribers: hiraditya, mcrosier, sanjoy.

This merges two complex CRTP mixins for the common "call" logic and
common operand bundle logic into a single, normal base class of
`CallInst` and `InvokeInst`. Going forward, users can typically
`dyn_cast<CallBase>` and use the resulting API. No more need for the
`CallSite` wrapper. I'm planning to migrate current usage of the wrapper
to directly use the base class and then it can be removed, but those are
simpler and much more incremental steps. The big change is to introduce
this abstraction into the type system.

I've tried to do some basic simplifications of the APIs that I couldn't
really help but touch as part of this:

- I've tried to organize the attribute API and bundle API into groups to make understanding the API of `CallBase` easier. Without this, I wasn't able to navigate the API sanely for all of the ways I needed to modify it.
- I've added what seem like more clear and consistent APIs for getting at the called operand. These ended up being especially useful to consolidate the *numerous* duplicated code paths trying to do this.
- I've largely reworked the organization and implementation of the APIs for computing the argument operands as they needed to change to work with the new subclass approach.

To minimize any cost associated with this abstraction, I've moved the
operand layout in memory to store the called operand last. This makes
its position relative to the end of the operand array the same,
regardless of the subclass. It should make it much cheaper to reference
from the `CallBase` abstraction, and this is likely one of the most
frequent things to query.

We do still pay one abstraction penalty here: we have to branch to
determine whether there are 0 or 2 extra operands when computing the end
of the argument operand sequence. However, that seems both rare and
should optimize well. I've implemented this in a way specifically
designed to allow it to optimize fairly well. If this shows up in
profiles, we can add overrides of the relevant methods to the subclasses
that bypass this penalty. It seems very unlikely that this will be an
issue as the code was *already* dealing with an ever present abstraction
of whether or not there are operand bundles, so this isn't the first
branch to go into the computation.

I've tried to remove as much of the obvious vestigial API surface of the
old CRTP implementation as I could, but I suspect there is further
cleanup that should now be possible, especially around the operand
bundle APIs. I'm leaving all of that for future work in this patch as
enough things are changing here as-is.

One thing that made this harder for me to reason about and debug was the
pervasive use of unsigned values in subtraction and other arithmetic
computations. I had to debug more than one unintentional wrap. I've
switched a few of these to use `int` which seems substantially simpler,
but I've held back from doing this more broadly to avoid creating
confusing divergence within a single class's API.

I also worked to remove all of the magic numbers used to index into
operands, putting them behind named constants or putting them into
a single method with a comment and strictly using the method elsewhere.
This was necessary to be able to re-layout the operands as discussed
above.


Repository:
  rL LLVM

https://reviews.llvm.org/D54788

Files:
  llvm/include/llvm/IR/CallSite.h
  llvm/include/llvm/IR/InstrTypes.h
  llvm/include/llvm/IR/Instructions.h
  llvm/lib/IR/Instructions.cpp
  llvm/lib/IR/Verifier.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54788.174900.patch
Type: text/x-patch
Size: 80542 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181121/a3eff114/attachment.bin>