[PATCH] D46695: [RFC] [Patch 1/3] Add a new class of predicates for variant scheduling classes.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb marked 2 inline comments as done.
andreadb added inline comments.


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:27
+//    CheckRegOperand<0>,
+//    MCPredicateNot<CheckRegOperandValue<0, LR>>]>;
+//
----------------
Ouch... this should have been "CheckNot", and not MCPredicateNot.
I will fix it.


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:73
+// A generic machine instruction predicate.
+class MCPredicate;
+
----------------
RKSimon wrote:
> Should this be more specific and called MCInstPredicate? I have no strong preference tbh
I will change it.


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:84
+// set of opcodes.
+class CheckNot<MCPredicate P> : MCPredicate {
+  MCPredicate Pred = P;
----------------
RKSimon wrote:
> What should be the rules on naming classes such as this? As Tablegen doesn't have any concept of namespace scope should all of these have a MC prefix or something?
That is a good question. The inability to use namespaces can be quite annoying limitation.
Most of these .td files end up being mass-included into a single entrypoint target .td file.
The probability of name clashes is not high, but not even zero.

I have been thinking about adding a prefix for those class names. However, it is much simpler and it reads nicely if we don't add an MC prefix.

That being said, I am open to suggestions. I am not very good with names..
If people have better ideas for names than I am happy to change them.


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:153
+// MCInst.
+class NonPortableCheck<string Code> : MCPredicate {
+  string CodeBlock = Code;
----------------
RKSimon wrote:
> AFAICT all the other MCPredicate child classes start with Check - CheckNonPortable?
Right. I will change it.


https://reviews.llvm.org/D46695