This is an archive of the discontinued LLVM Phabricator instance.

[Tablegen][MCInstPredicate] Removed redundant template argument from class TIIPredicate, and implemented verification rules for TIIPredicates in CodeGenSchedule.cpp.
ClosedPublic

Authored by andreadb on Aug 14 2018, 8:06 AM.

Details

Summary

This patch removes redundant template argument TargetName from TIIPredicate.
Tablegen can always infer the target name from the context. So we don't need to force users of TIIPredicate to always specify it.

This allows us to better modularize the tablegen class hierarchy for so-called "function predicates". class FunctionPredicateBase has been added; it is currently used as a building block for TIIPredicates. However, I plan to reuse that class to define other function predicates classes (i.e. not just TIIPredicates).
For example, this can be a first step towards implementing proper support for dependency breaking instructions in tablegen.

This patch also adds a verification step on TIIPredicates in tablegen.
We cannot have multiple TIIPredicates with the same name. Otherwise, this will cause build errors later on, when tablegen'd .inc files are included by cpp files and then compiled.

For example, if I add the following conflicting TIIPredicate at the end of X86SchedPredicates.td

def : TIIPredicate<"isThreeOperandsLEA", MCReturnStatement<TruePred>>;

tablegen (-gen-instr-info) now generates the following error:

Included from /home/andrea/llvm/lib/Target/X86/X86.td:405:
/home/andrea/llvm/lib/Target/X86/X86SchedPredicates.td:58:1: error: TIIPredicate isThreeOperandsLEA is multiply defined.
def : TIIPredicate<"isThreeOperandsLEA", MCReturnStatement<TruePred>>;
^
Included from /home/andrea/llvm/lib/Target/X86/X86.td:405:
/home/andrea/llvm/lib/Target/X86/X86SchedPredicates.td:55:1: note:  Previous definition of isThreeOperandsLEA was here.
def IsThreeOperandsLEAFn :
^
Included from /home/andrea/llvm/lib/Target/X86/X86.td:405:
/home/andrea/llvm/lib/Target/X86/X86SchedPredicates.td:58:1: error: Found conflicting definitions of TIIPredicate.
def : TIIPredicate<"isThreeOperandsLEA", MCReturnStatement<TruePred>>;
^

Please let me know if okay to commit.

Thanks,

  • Andrea

Diff Detail

Event Timeline

andreadb created this revision.Aug 14 2018, 8:06 AM
mattd accepted this revision.Aug 14 2018, 9:58 AM

I don't see anything wrong with this. I'm all for better modularity with the tablgen "function predicates"; LGTM.

This revision is now accepted and ready to land.Aug 14 2018, 9:58 AM
andreadb updated this revision to Diff 160643.Aug 14 2018, 11:13 AM

Thanks for the review Matt.

I noticed that I forgot to mark the new method 'const'. So I have decided to update the code review before committing it.

Changes are:

  • s/checkMCPredicates/checkMCInstPredicates
  • add "const" to method checkMCInstPredicates
  • add "const" to the Record* in the foreach loop

The rest should be identical.
If okay for you, then I would commit this version.

Sorry for the confusion and thanks in advance.

-Andrea

Thanks for the review Matt.

I noticed that I forgot to mark the new method 'const'. So I have decided to update the code review before committing it.

Even better. LGTM.

This revision was automatically updated to reflect the committed changes.