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

Mon Jun 14 03:27:47 PDT 2021

andreadb added a comment.

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

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

In my code, I tend to use `private` instead of `protected` whenever possible. This is just my coding style though. I don't think that LLVM imposes a particular coding style for it. Since mca has been mainly written by me, you'll find out that `protected` is almost never used in practice.

That being said, it is fine for you to use `protected` for your new class. It is your code, and -strictly speaking - there is nothing fundamentally wrong with it.

>> 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 for when/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 refer to 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)?

Yes please.

In case, any X86 specific changes should go into a separate patch.

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

I don't like the idea of using your new framework to workaround problems/bugs that are not in llvm-mca.

If you think that there is a genuine bug in their instruction definitions, then please raise  a bug for it.

We should never "fix" non-mca issues within mca. This is true in general, and it doesn't only apply to mca. If there is a bug in someone else's code, we create a bug report for it, and optionally we send a patch for it. We don't pretend that the issue doesn't exist, and (worse) hack some workarounds for those bugs in our code.

So, I strongly recommend to raise bugs for all these problems you have encountered when testing the AMDGPU models.
If there is a reason why there is a write to `M0`, then we can consider implementing a solution based on your new class/post-processing step.
If instead it that was an oversight, then it will be fixed outside of mca without the need for a custom hack in mca.

> 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? **Edit** Actually I just tried this, and there are other assertions that would need to be changed as well (one from `RegisterFile::removeRegisterWrite` and one from `RegisterFile::onInstructionExecuted`) so maybe this isn't a great option.
>
>   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)  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.
>
> If none of these options are desirable, another option could be to have a new method `InstrPostProcess::skipRegDef()` that gets called from `InstrBuilder::createInstruction()`:
>
>   // Initialize writes.
>   unsigned WriteIndex = 0;
>   for (const WriteDescriptor &WD : D.Writes) {
>     RegID = WD.isImplicitWrite() ? WD.RegisterID
>                                  : MCI.getOperand(WD.OpIndex).getReg();
>     // Check if this is a optional definition that references NoReg.
>     if (WD.IsOptionalDef && !RegID) {
>       ++WriteIndex;
>       continue;
>     }
>   
>     assert(RegID && "Expected a valid register ID!");
>     NewIS->getDefs().emplace_back(WD, RegID,
>                                   /* ClearsSuperRegs */ WriteMask[WriteIndex],
>                                   /* WritesZero */ IsZeroIdiom);
>     ++WriteIndex;
>   }
>
> To
>
>   // Initialize writes.
>   unsigned WriteIndex = 0;
>   for (const WriteDescriptor &WD : D.Writes) {
>     RegID = WD.isImplicitWrite() ? WD.RegisterID
>                                  : MCI.getOperand(WD.OpIndex).getReg();
>     // Check if this is a optional definition that references NoReg.
>     if ((WD.IsOptionalDef && !RegID) || IPP->skipRegDef(WD, MCI)) {
>       ++WriteIndex;
>       continue;
>     }
>   
>     assert(RegID && "Expected a valid register ID!");
>     NewIS->getDefs().emplace_back(WD, RegID,
>                                   /* ClearsSuperRegs */ WriteMask[WriteIndex],
>                                   /* WritesZero */ IsZeroIdiom);
>     ++WriteIndex;
>   }
>
> The more I think about it, the more I like the last option best. What do you think?

Assuming that it always works, Option 2. seems like the right way to go. A Write to an invalid registers should always be ignored.
That being said, let's wait to see if the issue with the write to `M0` can be fixed in a different way.
If not, then it is OK to add a post-processing step for it in mca. But that would go into a separate patch anyway.

-Andrea

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104149