[PATCH] D117451: [MCA] Proposing the InstrPostProcess:modifyInstrDesc() method.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In D117451#3309206 <https://reviews.llvm.org/D117451#3309206>, @holland11 wrote:

> In D117451#3307468 <https://reviews.llvm.org/D117451#3307468>, @andreadb wrote:
>
>> Hey Patrick,
>>
>> Are there any updates on this patch?
>
> Yes, sorry about the delay. I've been meaning to come here to ask for your input on how to go about restricting the API.
>
>   bool Modified = IPP.modifyInstrDesc(*ID, MCI);
>
> <snip>
>
> What I'd like to do is make it so that IPP has the ability to modify everything except the `Writes` and `Reads` vectors. Ideally, the `WriteDescriptor` and `ReadDescriptor` classes would have a new attribute (`Ignored`) that can be set to `true` by IPP, but that would be the only thing that IPP could do to those vectors.
>
> I can think of several ways to achieve the above, but none of them are particularly elegant and so I've just been trying to think a little bit each day to see if I can come up with a better solution. Do you have any ideas?

The way how I see it, is that method `postProcessInstruction` has been conveniently designed to allow for maximum flexibility. Targets are basically free to modify whatever aspect they want of the mca intermediate form. From a practical perspective, this is both great and scary; we are essentially exposing all the implementation details to the users. In terms of design, that is always a questionable choice.

Method `postProcessInstruction` is just a generic entrypoint for some custom/target specific logic. There is no structure and no emphasis to what steps are performed by the algorithm.
Introducing a more structured/restrictive API would require a more formal definition of what the post-processing stage does. I am afraid that there is no escape from this.

It essentially means that the sequence of steps performed by the post-processing logic must be defined in a target independent way. What I am suggesting is implementing some sort of policy base design, where the main algorithm essentially describes a workflow, but each step can be customised by Targets by overriding specific hooks.

Understanding how to structure the workflow is probably the hardest part. At the very least, it requires good knowledge of all the requirements derived from known use cases.

As far as I am aware of, these have always been the three major requirements:

1. Add the ability to model special target specific hazards. This was solved by introducing custom `MCAOperand` operands to `mca::Instruction`, and adding a `CustomBehaviour` class.
2. Add the ability to toggle some instruction flags ON/OFF. For example, some Targets may want to be able to set flag `RetireOOO`.
3. Add the ability to disable individual read/write operands.

Assuming that my understanding is correct, this is how a possible API for the Target policy might look like:

  virtual void populateCustomOperands(SmallVectorImpl<MCAOperand> &InVec, const MCInst &MCI) { }
  virtual void toggleFlags(mca::InstructionFlags& Flags, const MCInst &MCI) { }
  virtual bool shouldBeDisabled(mca::ReadState& Read, const MCInst &MCI) { return false; };
  virtual bool shouldBeDisabled(mca::WriteState& Write, const MCInst &MCI) { return false; };

Here, every functionality implements an individual step in the post-processing algorithm. Targets that want to customise a specific step, only need to override the corresponding method(s) of the post-processing interface. For example: if a Targets wants to be able to toggle `mca::Instruction` flags, then it needs to override method `toggleFlags`; etc.

This is how a process might look like:

   postProcessInstruction(std::unique_ptr<Instruction> &Inst, const MCInst &MCI)
  {
  
      // Step 1. Populate custom operands.
      // NOTE1: We need to convert the std::vector of MCAOperand into a SmallVector.
      // NOTE2: we need a getMCAOperands() to obtain a reference to the MCAOperand vector in mca::Instruction. 
  
      populateCustomOperands(Inst->getMCAOperands(), const MCInst &MCI);
  
     // Step 2. Add the ability to manipulate instruction flags.
     // NOTE: We need to move all the flags into their own struct. This example assumes the presence of 
     // a struct named `mca::InstructionFlags`.
     //
     // NOTE2: Changing flags at InstrDesc level might be problematic since it has the potential of changing the
     // semantic of a class of instruction (and not just those with a specific opcode).
     // Consider adding a copy of the flags in every mca::Instruction. The mca pipeline would then check the
     // copy in mca::InstructionBase, rather than the default values from InstrDesc.
  
      toggleFlags(Inst->getFlags(), MCI);
     
      // Step 3. Disable specific def/uses.
      for(auto &def : Inst->getDefs())
        if (shouldDisable(def, opcode))
          disable(def);
       
      for each(use : Inst->getUses())
        if (shouldDisable(use, opcode))
          disable(use)

PROS & CONS:

Introducing a more restrictive API essentially requires that we standardise the post-processing workflow. It is basically some sort of strategy pattern, where each Target provides its own policy for implementing individual steps.

This is obviously far less flexible that what we had before. However, it may be argued that previously, Targets had potentially way too much freedom.
For some proprietary targets this new design might not be good enough. That being said, for most of those targets it would be just a matter of adding yet-another step to the sequence performed by the post-processing stage.

I have also added some notes in my pseudo-code to highlight some issues/things that need to be addressed for this patch to work.
My biggest concern is about how flags are manipulated. We should probably considering copying flags into `mca::InstructionBase`. That way, we avoid manipulating `InstrDesc` directly. This would also require that we fix every direct reference to InstrDesc flags from the various mca pipeline stages. Hopefully this would be simple to fix in case.

In general, I can't say for sure that what I have suggested here is a good step forward. However, I think that it would be an improvement in general.

What do you think? Is this a too crazy idea? I hope not :)

-Andrea


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117451