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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 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;
+}
----------------
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. 


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

https://reviews.llvm.org/D89458