[PATCH] D104149: [MCA] Adding the CustomBehaviour class to llvm-mca

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In D104149#2815523 <https://reviews.llvm.org/D104149#2815523>, @holland11 wrote:

> @andreadb I appreciate all of the suggestions. They all make sense to me and I'll get started on implementing them.
>
> 1. Once I've made the changes, would it be better for me to update the diff within this post, or create a new post with the same title/body (but with a new diff)?

You don't need to create a new review. You can always update the review description if you want, and keep everything as part of this same review.

> 2. Your concerns about the MCAOperand list are completely reasonable. I'll get started on making the list optional and only storing the operands that the target's CB requests. What do you think would be the best way to make it 'optional' (with respect to the actual datatype of the MCAOperand list within the mca::Instruction class)? Would making it a std::vector that defaults to a size of 0 be smart or can you think of a better way?

I would simply use a std::vector. That way, it would start empty (which is what we want most of the times).

> What I'm thinking is that within the CustomBehaviour.h file, there will be another class `InstrPostProcess` (open to better names). This class will have a public method `processInstruction()` (modify may be a better word). The generic class's method won't do anything, but targets who implement their own CB will also implement their own `InstrPostProcess` where they can choose to override the `processInstruction()` method. After the `MCInst` is lowered to `mca::Instruction`, but before we add it to the `LoweredSequence`, we will call `InstrPostProcess::processInstruction()`. This method will take both the `MCInst` and `mca::Instruction` and will make any modifications / additions to the `mca::Instruction`.

Sound good. Let say that one day we decide to add support for MachineInstr too, do you think it would be easy to extend the CustomBehaviour API? Basically, I would go for the most reusable design possible. Basically, we should value both simplicity (of the design) and reusability (when possible).

> This way, for targets that don't have a CB or don't need any modifications to the `mca::Instruction` (such as storing specific operands), the `processInstruction()` method can trivially return without doing anything. And for targets who do want to make modifications to the `mca::Instruction`, they can have a `switch` statement within the `processInstruction()` to only modify the desired instructions.

Awesome.

> I mentioned in the original post that CB is currently only designed to handle adding dependencies, but not removing them. As a bonus with this design, we'd get an easy way to remove undesired dependencies (on top of the original intention to only store what we need).

Great.
However, let's just pretend for now that this is all about adding new dependencies.
We can re-evaluate the importance of this particular use case in future if we find that it is required by some code.

> **Edit**: I just did a quick, rough, and untested re-design for the MCAOperand issue if you want a more explicit look at what this plan looks like (don't feel any pressure to look at it though, I'm just sharing in case you're curious) https://pastebin.com/rL7w0WC4

For what I can see, it looks good!

Only a couple of minor nits:

- Protected fields of `InstrPostProcess` should be private (even if at the cost of adding const& getters).
- Do we actually even need to add a derived class for x86? For X86 the base class seems already enough.

Thanks
-Andrea


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104149