This is an archive of the discontinued LLVM Phabricator instance.

[tablegen] Extract portions of AsmMatcherEmitter for re-use by another generator. NFC.
ClosedPublic

Authored by dsanders on Oct 14 2016, 6:36 AM.

Details

Summary

This change is preparation for a change that will allow targets to verify that the instructions
they emit meet the predicates they specify. This is useful to ensure that C++
legalization/lowering/instruction-selection doesn't incorrectly select code for a different
subtarget than intended. Such cases are not caught by the integrated assembler when emitting
instructions directly to an object file.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders updated this revision to Diff 74674.Oct 14 2016, 6:36 AM
dsanders retitled this revision from to [tablegen] Extract portions of AsmMatcherEmitter for re-use by another generator. NFC..
dsanders updated this object.
dsanders added a subscriber: llvm-commits.

ping

This is a pre-requisite of D25618 which has been LGTM'd

qcolombet accepted this revision.Nov 10 2016, 12:21 PM
qcolombet added a reviewer: qcolombet.
qcolombet added a subscriber: qcolombet.

Hi Daniel,

LGTM. Couple of nitpicks below.

Cheers,
-Quentin

utils/TableGen/AsmMatcherEmitter.cpp
1416 ↗(On Diff #74674)

I would rather stick to the explicit type here.

utils/TableGen/SubtargetFeatureInfo.cpp
30 ↗(On Diff #74674)

Just a thought, shouldn't this string be an argument?
Indeed, if we really want to reuse the functionality we shouldn't bake assembler specific stuff in.

I'm fine with a comment saying that if we want to extend the reusability we would need to pass an additional parameter.

utils/TableGen/SubtargetFeatureInfo.h
42 ↗(On Diff #74674)

Add comments on what TargetName and ClassName are used.
Also, don't copy the string -> StringRef or const &

This revision is now accepted and ready to land.Nov 10 2016, 12:21 PM
dsanders updated this revision to Diff 77800.Nov 14 2016, 6:13 AM
dsanders marked 2 inline comments as done.
dsanders edited edge metadata.
  • explicit type
  • std::string->StringRef
  • Comment on parameters and AssemblerMatcherPredicate string
dsanders updated this revision to Diff 77801.Nov 14 2016, 6:15 AM
dsanders marked an inline comment as done.

Be slightly clearer in a comment

dsanders added inline comments.Nov 14 2016, 6:17 AM
utils/TableGen/SubtargetFeatureInfo.cpp
30 ↗(On Diff #74674)

I agree that the name 'AssemblerMatcherPredicate' doesn't make as much sense as it used to but not because of the 'Assembler' part. The new use is still part of an assembler, it just receives the IR directly instead of parsing it from text. The bit that I think may be dubious is the 'Matcher' part because no matching occurs in the new use. At this point, I think it's really a 'MCPredicate' but I'm not convinced it's worth renaming just yet.

I'll add the comment in case someone adds other kinds of predicates in the future.

dsanders closed this revision.Nov 15 2016, 2:00 AM
This revision was automatically updated to reflect the committed changes.