Page MenuHomePhabricator

[RFC][TableGen/RISCV] Support combining AssemblerPredicates with ORs
Needs ReviewPublic

Authored by simoncook on Mon, Feb 10, 9:07 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
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, I
propose extending the supported syntax in the "AssemblerCondString" to
create constraints which meet this need. Since a single comma is used
to mean and, I propose using a vertical bar to mean or, since this
matches the mental model when glancing at predicate definitions. I have
modified TableGen's AsmWriterEmitter, FixedLenDecoderEmitter and
SubtargetFeatureInfo's computeAssemblerAvaiableFeatures emitter to
emit code which achieves this result.

(As an aside, this includes a proposed method of consuming sets of
features or'd together in a set of conditions that are typically all
combined with ands).

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

If this seems like a sensible feature, I intend to update this review
with some unit tests (and submit the RISCV change separately), but for
now showing the two together demonstates the type of thing I would like
to do.

Diff Detail

Event Timeline

simoncook created this revision.Mon, Feb 10, 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
667

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
128

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

simoncook marked an inline comment as done.Wed, Feb 12, 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.Thu, Feb 13, 5:46 AM

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

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

Add AsmWriter test

simoncook updated this revision to Diff 244464.Thu, Feb 13, 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.