[PATCH] D89458: [ARM][SchedModels] Convert IsPredicatedPred to MCSchedPredicate

Andrea Di Biagio via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 16 01:46:37 PDT 2020


andreadb added inline comments.


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:183-186
+bool ARM_MC::isPredicated(const MCInst &MI) {
+  int PredOpIdx = MI.getDesc()->findFirstPredOperandIdx();
+  return PredOpIdx != -1 && MI.getOperand(PredOpIdx).getImm() != ARMCC::AL;
+}
----------------
evgeny777 wrote:
> andreadb wrote:
> > It is unfortunate that the MCSchedPredicate framework is not expressive enough to let users define predicates on MCInstrDesc.
> > 
> > Your idea of adding an extra field (a MCInstrDesc pointer) to MCInst does solve the issue.
> > To be honest, at the moment I don't think that there are other ways to fix this issue. The only alternative is to expand/improve the MCSchedPredicate framework. However, it is not a simple task, and it would requires modifications to the SubtargetEmitter, the InstructionInfoEmitter and the PredicateExpander.
> > The only alternative is to expand/improve the MCSchedPredicate framework. However, it is not a simple task, and it would requires modifications to the SubtargetEmitter, the InstructionInfoEmitter and the PredicateExpander.
> 
> I tried adding extra parameter (MCInstrInfo) to resolveVariantSchedClass and resolveVariantSchedClassImpl, updating SubtargetEmitter and all callers (llvm-mca, llvm-exegesis, etc) respectively. However I didn't like the result much due to
> 
> - lots of code modification
> - all MC predicates will have two parameters, which codegen predicates have only one, so PredicateExpander should be modified (like you said)
> - most of MC predicates likely won't need MCInstrDesc.
> 
> Looking at MachineInstr I found getDesc/setDesc and decided to use similar approach for MCInst. 
Yeah.

I agree with you that having a pointer to the instruction descriptor is quite convenient. It simplifies some interactions and avoids that we always keep around a reference to the MCInstrInfo. So, if other people are happy with it, then I am OK too.

Yesterday I was trying to prototype something to workaround that issue, and I think I ended up doing exactly the same modifications (i.e. adding an extra MCInstrInfo* operand to `resolveVariantSchedClass`).
However I think (although I didn't test it yet) that you can probably implement something simpler by introducing only a new predicate, and not having to touch existing predicates. 

The main problem in your case was that  `CheckFunctionPredicate` is not very good because it assumes a single operand in input to the function (i.e. a MachineInstr/MCInst operand).

We could introduce a `CheckFunctionPredicateWithMCII<string MCInstFn, string MachineInstrFn>`

which is similar in principle to `CheckFunctionPredicate`, but it assumes that the MachineInstrFn is a method of TII, and MCInstrFn is just a normal function that also takes a pointer to MCII in input. So, something that expands to either of:
 -  `TII->MachineInstrFn(MI);`
 - `MCInstFn(MI, MCII)`.

Basically, same idea, but extra MCII operand.
That would obviously require that MCII is accessible somehow (hence the need of that change to the signature of resolveVariantSchedClass - so that MCII is always available).

If that works, then your new function would be become like this:
```
bool ARM_MC::isPredicated(const MCInst &MI, const MCInstrInfo* MCII) {
  const MCInstrDesc& Desc = MCII->get(MI.getOpcode());
  int PredOpIdx = Desc.findFirstPredOperandIdx();
  return PredOpIdx != -1 && MI.getOperand(PredOpIdx).getImm() != ARMCC::AL;
}
```

In conclusion:
I am not against the idea of adding an extra field to MCInst since it may be useful in general (maybe not just for mca).
It definitely simplifies writing portable predicates that can be used by mca. It is a pragmatic approach after all, and - as you wrote - we already do something similar in MachineInstr.

An alternative approach, would require a change to the resolveVariant signature, and the introduction of a new predicate. As you wrote , the downside is that it introduces code modifications in a few places (at least in the SubtargetEmitter, MCSchedule.h (functionalities that need to resolve scheduling classes to compute latency/throughput), and llvm-mca/exegesis (since those obviously need to resolve latency/throughput). That being said, those are mostly mechanical changes (I personally don't think it is a problem to change those tools).



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

https://reviews.llvm.org/D89458



More information about the llvm-commits mailing list