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

[Patrick Holland via Phabricator via llvm-commits]

holland11 added a comment.

> Protected fields of InstrPostProcess should be private (even if at the cost of adding const& getters).

Does this hold true in general for objects that get stored in base classes, but used in derived classes? I also have the `MCSubtargetInfo`, `SrcManager`, and `MCInstrInfo` objects within the base CB as protected. Should I change them to private and then use getters for the derived classes to access them?

> Do we actually even need to add a derived class for x86? For X86 the base class seems already enough.

We probably don't need it. I was originally going to make an empty derived class for every upstream target just so they'd have all their cmake stuff setup already if people wanted to add to them. But then I just decided to keep two of them so that it would be easy for people to reference how they are setup. Though, keeping the X86 one seems a bit misleading considering that CB doesn't even play a part in the out-of-order pipeline yet. Do you think I should remove it (the X86 derived class)?

> 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.

I'm really tempted to use this altered design to fix the `ds_read` issue that I described at the end of the original post. It would be a much cleaner example use case than the `s_waitcnt` 'fix' (although it wouldn't use the actual CB class at all so it wouldn't be an example of CB directly).

>From a high level, all I need to do is remove the `WriteState` that refers to the `M0` register. From a lower level, this `WriteState` exists in the `mca::Instruction` as part of `SmallVector<WriteState, 2> Defs;`. From looking at the way the `WriteState` objects interact with the `RegisterFile` (most notably, what happens in both pipelines when an instruction gets dispatched and needs to add its defs to the register file), I could either **(1)** remove the `WriteState` from the `SmallVector`, **(2)** set the `RegisterID` of the `WriteState` to 0, or **(3)** set the `Latency` of the `WriteState` to 0.

1. As far as I can tell, there is no way to remove an element from a `SmallVector`. I could maybe build a new `SmallVector` without the specific `WriteState`, but this seems less than ideal.

2. This would cause an assertion to fail within `RegisterFile::addRegisterWrite`. Would it be alright to change this to an if statement that exits the function instead?

  void RegisterFile::addRegisterWrite(WriteRef Write,
                                      MutableArrayRef<unsigned> UsedPhysRegs) {
    WriteState &WS = *Write.getWriteState();
    MCPhysReg RegID = WS.getRegisterID();
    assert(RegID && "Adding an invalid register definition?");

To

  void RegisterFile::addRegisterWrite(WriteRef Write,
                                      MutableArrayRef<unsigned> UsedPhysRegs) {
    WriteState &WS = *Write.getWriteState();
    MCPhysReg RegID = WS.getRegisterID();
    if (RegID == 0)  return;



3. This is my least desired route since I don't fully understand the ramifications that it would have and doesn't seem as 'explicit' as far as removing a def from an instruction goes.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104149