This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] Add useDeprecatedPositionallyEncodedOperands option.
ClosedPublic

Authored by jyknight on Sep 16 2022, 1:18 PM.

Details

Summary

The existing undefined-bitfield-to-operand matching behavior is very
hard to understand, due to the combination of positional and named
matching. This can make it difficult to track down a bug in a target's
instruction definitions.

Over the last decade, folks have tried to work-around this in various
ways, but it's time to finally ditch the positional matching. With
https://reviews.llvm.org/D131003, there are no longer cases that
_require_ positional matching, and it's time to start removing usage
and support for it.

Therefore: add a (default-false) option, and set it to true only in
those targets that require positional matching today. Subsequent
changes will start cleaning up additional in-tree targets.

NOTE TO OUT OF TREE TARGET MAINTAINERS:

If this change breaks your build, you may restore the previous
behavior simply by adding:

let useDeprecatedPositionallyEncodedOperands = 1;

to your target's InstrInfo tablegen definition. However, this is
temporary -- the option will be removed in the future.

If your target does not set 'decodePositionallyEncodedOperands', you
may thus start migrating to named operands. However, if you _do_
currently set that option, I recommend waiting until a subsequent
change lands, which adds decoder support for named sub-operands.

Diff Detail

Event Timeline

jyknight created this revision.Sep 16 2022, 1:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 1:18 PM
jyknight requested review of this revision.Sep 16 2022, 1:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 1:18 PM
MaskRay added inline comments.Sep 18 2022, 10:51 PM
llvm/include/llvm/Target/Target.td
1052

Perhaps use a TODO as a minder that the support will be removed.

llvm/test/TableGen/InsufficientPositionalOperands.td
33

Add CHECK-NEXT: note: Dumping record for previous error: then use --implicit-check-not=error:

Q: by changing $xd, do we lose test coverage by not testing an undefined operand name?

llvm/utils/TableGen/CodeEmitterGen.cpp
53–54

addCodeToMergeInOperand now that the signature has changed

119

Does this need a TODO that this branch will be removed?

133

Drop the change to this code block

165

Since PrintError takes a Twine. You may remove E and S and just construct the long Twine as an argument

jyknight updated this revision to Diff 461541.Sep 20 2022, 5:33 AM
jyknight marked 6 inline comments as done.

Address review feedback. Also tweak error reporting to improve consistency for the "too few operands" case.

llvm/test/TableGen/InsufficientPositionalOperands.td
33

Done, and good point about losing test coverage. Switched this test to useDeprecatedPositionalOperands to preserve testing the positional error, and another test added for when positional matching is off.

MaskRay accepted this revision.Sep 20 2022, 9:36 AM

Looks great!

llvm/utils/TableGen/DecoderEmitter.cpp
2134–2159

Does early return pattern apply?

This revision is now accepted and ready to land.Sep 20 2022, 9:36 AM
This revision was landed with ongoing or failed builds.Sep 24 2022, 6:41 AM
This revision was automatically updated to reflect the committed changes.