[PATCH] D104268: [ptr_provenance] Introduce optional ptr_provenance operand to load/store

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added a comment.

In D104268#3107611 <https://reviews.llvm.org/D104268#3107611>, @asbirlea wrote:

> - Clarified item: If a load/store has 2/3 operands out of which only 1/2 are used, these operands are stored in the last position(s). So when adding provenance, the current operands need to be shifted to the first position(s) and the provenance added as the last operand.

There are a number of implementations possible for adding an optional argument. They all share that at some moment, they need to know the actual
number of arguments in use. Depending on the implementation, those moments can differ.

It is also important to know that the operands are stored _before_ the `this` pointer. (See implementation of `getIntrusiveOperands()`)
In order to get to the location of the first operand, we subtract `NumUserOperands` from `this`.

The original (//obsolete//) implementation (**variant 0**) always reserves the optional argument.  The reserved amount of operands is stored in NumUserOperands,
and an extra bit 'NumUserOperandsDelta' was used when we need to know the actual number of operands.

The main issue with variant 0, is that it introduces and extra bit extract + addition in a number of common operations. Not just for load/store instructions, but for all instructions.
This has a measurable compile time impact :(

The split-up compile time impact (and patches) of variant0 can be found here:
https://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&branch=dobbelaj-snps/perf/test_instructions_20200907-01

So, I came up with 2 other variants that can be seen here:
https://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&branch=dobbelaj-snps/perf/test_instructions_20200907-02

**variant 1**: This variant also always reserves the optional argument. But now, NumUserOperands tracks the actual number of operands in use.
As `getIntrusiveOperands()` uses NumUserOperands() to find back the location of the first operand, we now have a problem when introducing (or removing)
the ptr_provenance operand: when changing NumUserOperands, //suddenly all operands are shifted//.  Because of this behavior, the code for
adding (removing) the operand is slightly more complex, as it is also moving around the operands //to compensate for the shifting behavior//.

The big benefit is that we now only pay this penalty when using the new operand. So, **variant 1** only has a minor compile time impact.

Then I also tried out **variant 2**. This is similar to **variant 1**, but they introduce a separate version of Load (and Store) with or without memory
space for the optional argument. The main benefit is that, if the ptr_provenance is never used, no space is reserved for it. But, the initial switching from
a 'standard' Load/Store to a 'Load/Store with optional ptr_provance support' is a lot higher and maintaining the correct usage of the instructions is also
more complex, which tend to result in more subtle errors.

Looking at the compile time and heap impact, and the potential maintenance issues, I settled on using **variant 1**, which is also the version implemented in this review.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104268/new/

https://reviews.llvm.org/D104268