Page MenuHomePhabricator

[TableGen] Support combining AssemblerPredicates with ORs
ClosedPublic

Authored by simoncook on Feb 10 2020, 9:07 AM.

Details

Summary

For context, the proposed RISC-V bit manipulation extension has a subset
of instructions which require one of two SubtargetFeatures to be
enabled, 'zbb' or 'zbp', and there is no defined feature which both of
these can imply to use as a constraint either (see comments in D65649).

AssemblerPredicates allow multiple SubtargetFeatures to be declared in
the "AssemblerCondString" field, separated by commas, and this means
that the two features must both be enabled. There is no equivalent to
say that _either_ feature X or feature Y must be enabled, short of
creating a dummy SubtargetFeature for this purpose and having features
X and Y imply the new feature.

To solve the case where X or Y is needed without adding a new feature,
and to better match a typical TableGen style, this replaces the existing
"AssemblerCondString" with a dag "AssemblerCondDag" which represents the
same information. Two operators are defined for use with AssemblerCondDag,
"all_of", which matches the current behaviour, and "any_of", which adds
the new proposed ORing features functionality.

Changes to all current backends are mechanical to support the replaced
functionality, and are NFCI.

At this stage, it is illegal to combine features with ands and ors in
a single AssemblerCondDag. I suspect this case is sufficiently rare
that adding more complex changes to support it are unnecessary.

Diff Detail

Event Timeline

simoncook created this revision.Feb 10 2020, 9:07 AM

I can't see any issues with the logic of this implementation. It doesn't lend itself well to adapting it in the future if we want to be able to combine the operators, but I'd expect that would require a more disruptive rewrite so I'm not too concerned about that right now. See comments though.

llvm/include/llvm/MC/MCInstPrinter.h
133

Nitpick: Realign comments in this enum

llvm/lib/MC/MCInstPrinter.cpp
149

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.

This seems reasonable. Looking forward to seeing this fleshed out with tests.
I see no problem with not supporting both AND and OR at the same time for now, as that could be added later if the need becomes clear.

llvm/include/llvm/Target/Target.td
678

Nitpick: this could benefit from a deeper edit to better reflect the new semantics, in a clear way. Be sure to clarify that , and | are currently mutually exclusive.

llvm/utils/TableGen/SubtargetFeatureInfo.cpp
124

Better to put something that indicates that this isn't currently implemented, but could be supported.

simoncook marked an inline comment as done.Feb 12 2020, 9:06 AM

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.

llvm/lib/MC/MCInstPrinter.cpp
149

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?

simoncook updated this revision to Diff 244407.Feb 13 2020, 5:46 AM

Incorporate feedback, add test of disassembler and assembly matching test (asmwriter test is WIP)

simoncook updated this revision to Diff 244432.Feb 13 2020, 7:56 AM

Add AsmWriter test

simoncook updated this revision to Diff 244464.Feb 13 2020, 9:46 AM

Add test for RISCV Instruction Compression, so now all uses of AssemblerCondString are covered by this patch.

It's a logical addition to the semantics of features. Though at the moment RV is probably the only user of this addition, the eventual patch implementing it should not include the RV specific changes, which would be better in a separate patch.

It's a logical addition to the semantics of features. Though at the moment RV is probably the only user of this addition, the eventual patch implementing it should not include the RV specific changes, which would be better in a separate patch.

That’s my plan, they’re just currently put together to demonstrate. I’ll separate them before submitting (the RV part will move to the bitmanip MC patch)

We have a similar situation in the ARM backend, where there are a few move/load/store instructions operating on FP regs, which are available with either FeatureVFPv2_SP or HasIntegerMVEOps. We solved this by adding new target features for these common instructions, and adding dependencies from the other features to them, so that they get enabled whenever the "real" features are enabled. This works, but has the downside that clang now needs to disable these features when disabling the VFP/MVE features. This patch looks like a better solution to the problem, since clang doesn't need to know about the shared instructions.

simoncook updated this revision to Diff 246174.Feb 24 2020, 3:23 AM
simoncook retitled this revision from [RFC][TableGen/RISCV] Support combining AssemblerPredicates with ORs to [RFC][TableGen] Support combining AssemblerPredicates with ORs.
simoncook edited the summary of this revision. (Show Details)
simoncook set the repository for this revision to rG LLVM Github Monorepo.

Following feedback from @nhaehnle (http://lists.llvm.org/pipermail/llvm-dev/2020-February/139186.html) I have re-designed this to look more TableGen like (i.e. using DAGs instead of strings to describe the predicate). I have declared two operators any_of and all_of which I think read better than and and or in these cases when just looking at the target files.

  • I have removed the RISC-V bitmanip changes I want to support (I'll rebase D65649 on this to remove the circular dependency).
  • Added tests for existing functionality and new or functionality.
  • Changes to all backends are mechanical, no functional changes are intended, everything still passes with ninja check-llvm on my side.

I may need to improve some of the emitters I've touched, in particular RISCVCompressInstEmitter, but I think this is sufficient to carry on discussing the method of my proposed change.

nhaehnle accepted this revision.Mar 10 2020, 2:52 AM

Thank you for doing this, much appreciated. LGTM.

This revision is now accepted and ready to land.Mar 10 2020, 2:52 AM
lenary added inline comments.Mar 10 2020, 4:47 AM
llvm/include/llvm/Target/Target.td
667–675

This should document that you cannot do (any_of Pred1, (all_of Pred2, Pred3)), but you can do:

def Pred23 = AssemblerPredicate<(all_of Pred2, Pred3)>;
... = AssemblerPredicate<(any_of Pred1, Pred23)>;

It'd be nice if a single predicate wouldn't need all_of, but I don't feel strongly about it.

simoncook marked an inline comment as done.Mar 13 2020, 5:46 AM

It'd be nice if a single predicate wouldn't need all_of, but I don't feel strongly about it.

I was thinking about this when implementing the patch originally. I opted against that because it would be unlike other uses of dags in TableGen. I'm thinking in particular about RegisterClasses, where even for just a "stack pointer register class" we still do (add R2) in the definition.

simoncook retitled this revision from [RFC][TableGen] Support combining AssemblerPredicates with ORs to [TableGen] Support combining AssemblerPredicates with ORs.Mar 13 2020, 8:05 AM
This revision was automatically updated to reflect the committed changes.