[PATCH] D74338: [RFC][TableGen/RISCV] Support combining AssemblerPredicates with ORs

[Simon Cook via Phabricator via llvm-commits]

simoncook marked an inline comment as done.
simoncook added a comment.

Thanks for the feedback @luismarques and @lewis-revill.

I think regarding combining operators, it's still possible to do this by adding multiple `Predicates` on an Instruction definition, and these would still combine as an add (see my comment about `GORCI`), so the limitation is just about being in a single `AsmPredicate` definition. I agree that if we wanted to support this in a single `AsmPredicate`, then the change would be much more invasive, and since this appears to be the first instance that an or would be useful, combining ands and ors is probably sufficiently rare that refactoring it without a known use case may not be worth it; for now I'll document this limitation.



================
Comment at: llvm/lib/MC/MCInstPrinter.cpp:149
+    bool OrPredicateResult = false;
     if (llvm::all_of(Conds, [&](const AliasPatternCond &C) {
+          return matchAliasCondition(*MI, STI, MRI, OpIdx, M, C,
----------------
lewis-revill wrote:
> I don't know this code well enough to know if this is feasible but could this work by instead checking at this point if the pattern is an 'or' pattern, and selecting using 'any_of' if that's the case? We could then avoid adding the K_Or conditions.
It's not possible to do this in this case, since the tests here are made up of more than one predicate, you end up with a list of things which must all be true, but with some with this or case in the middle. As an example of the set of predicates that make it here (taken from `RISCVGenAsmWriter.inc`

```
    // (GORCI GPR:$rd, GPR:$rs, { 1, 1, 1, 1, 0 }) - 327
    {AliasPatternCond::K_RegClass, RISCV::GPRRegClassID},
    {AliasPatternCond::K_RegClass, RISCV::GPRRegClassID},
    {AliasPatternCond::K_Imm, uint32_t(30)},
    {AliasPatternCond::K_OrFeature, RISCV::FeatureExtZbb},
    {AliasPatternCond::K_OrFeature, RISCV::FeatureExtZbp},
    {AliasPatternCond::K_EndOrFeatures, 0},
    {AliasPatternCond::K_NegFeature, RISCV::Feature64Bit},
```

So in this case we only want to use this Asm string if the types match (the first three lines), and we have Zbb or Zbp in addition to not being 64-bit. Because of this I can't switch to using `any_of` because I'll match the wrong things. Adding the or features with the end of list marker meant I could keep this pattern and do a less invasive change. I suspect doing anything more complex might require a more in-depth change, I'm not sure if anyone who knows this part of the compiler might have other thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74338