Page MenuHomePhabricator

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

Authored by andreadb on May 10 2018, 8:23 AM.

Details

Summary

This patch is the first of a sequence of three patches described by the LLVM-dev RFC "MC support for variant scheduling classes".
http://lists.llvm.org/pipermail/llvm-dev/2018-May/123181.html

The goal of this patch is to introduce a new class of scheduling predicates for SchedReadVariant and SchedWriteVariant.

An MCSchedPredicate can be used instead of a normal SchedPredicate to model checks on the instruction (either a MachineInstr or a MCInst).
Internally, an MCSchedPredicate encapsulates an MCInstPredicate definition. MCInstPredicate allows the definition of expressions with a well-known semantic, that can be used to generate code for both MachineInstr and MCInst.

This is the first step toward teaching to tools like lllvm-mca how to resolve variant scheduling classes.

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb created this revision.May 10 2018, 8:23 AM
andreadb edited the summary of this revision. (Show Details)May 10 2018, 8:49 AM
mattd added a subscriber: mattd.May 10 2018, 9:46 AM
javed.absar added inline comments.May 10 2018, 3:12 PM
include/llvm/Target/TargetInstrPredicate.td
22 ↗(On Diff #146131)

Hi Andrea:
It would help immensely to have a simple example of usage documented here, illustrating how it all fits together

javed.absar added inline comments.May 10 2018, 3:20 PM
include/llvm/Target/TargetInstrPredicate.td
48 ↗(On Diff #146131)

Where is the check itself? Maybe the comment is not right or I am not getting this right.

andreadb added inline comments.May 11 2018, 6:12 AM
include/llvm/Target/TargetInstrPredicate.td
22 ↗(On Diff #146131)

You are right. I am going to upload a (more descriptive) new version of the patch.
I am also going to update the description.

andreadb updated this revision to Diff 146343.May 11 2018, 9:57 AM

Patch updated.

This new version of the patch improves the description of each MCPredicate class.
It also better describes how to use predicates, and how they are expanded by tablegen.
The new description lives in llvm/Target/TargetInstrPredicate.td.

andreadb edited the summary of this revision. (Show Details)May 11 2018, 9:57 AM

A few minor questions about naming conventions.

include/llvm/Target/TargetInstrPredicate.td
73 ↗(On Diff #146343)

Should this be more specific and called MCInstPredicate? I have no strong preference tbh

84 ↗(On Diff #146343)

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?

153 ↗(On Diff #146343)

AFAICT all the other MCPredicate child classes start with Check - CheckNonPortable?

A few minor questions about naming conventions.

For the record, I have no preference on the names.

andreadb marked 2 inline comments as done.May 11 2018, 10:42 AM
andreadb added inline comments.
include/llvm/Target/TargetInstrPredicate.td
27 ↗(On Diff #146343)

Ouch... this should have been "CheckNot", and not MCPredicateNot.
I will fix it.

73 ↗(On Diff #146343)

I will change it.

84 ↗(On Diff #146343)

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.

153 ↗(On Diff #146343)

Right. I will change it.

andreadb updated this revision to Diff 146395.May 11 2018, 12:56 PM
andreadb edited the summary of this revision. (Show Details)

Patch updated.

Addressed Simon's review comments.

javed.absar added inline comments.May 13 2018, 8:36 AM
include/llvm/Target/TargetInstrPredicate.td
24 ↗(On Diff #146395)

On the topic of names, would 'CheckAll' be shorter and better? I don't see the 'Of' adding more.

andreadb added inline comments.May 14 2018, 2:33 AM
include/llvm/Target/TargetInstrPredicate.td
24 ↗(On Diff #146395)

Sure. I will change it. Thanks.

andreadb updated this revision to Diff 146628.May 14 2018, 8:56 AM

Addressed review comments.

s/CheckAllOf/CheckAll
s/CheckAnyOf/CheckAny

RKSimon added inline comments.May 17 2018, 3:29 PM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

The use of 'Value' makes is it sound like we need the register to contain a certain value, not that it must be a certain register.

andreadb added inline comments.May 18 2018, 2:09 AM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

I am okay with bikeshedding names if people don't like them.

What if I change CheckRegOperand into CheckOperandIsRegister, and then rename CheckRegOperandValue to CheckRegOperand?

andreadb updated this revision to Diff 147481.May 18 2018, 5:14 AM
andreadb added reviewers: rengolin, ab.

Addressed review comment.

rengolin added inline comments.May 18 2018, 5:25 AM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

This would make git archaeology confusing... :)

RKSimon added inline comments.May 18 2018, 6:41 AM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

@rengolin Do you mean if the names have to change in the future after the patch is committed? A big concern is tablegen's lack of scoping might cause clashes in the future.

rengolin added inline comments.May 18 2018, 6:59 AM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

No, the proposal seem to be A -> B, C -> A; which will make confusing to compare patches on the two sides of the change regarding CheckRegOperand and what it means, but that's probably a minor concern.

Anyhow, I'm not trying to chime in on a bikeshedding argument. :)

andreadb added inline comments.May 18 2018, 7:10 AM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

If you mean D46697, then that patch is no longer valid. I am going to close it (or update it to reflect these last changes).

rengolin added inline comments.May 18 2018, 7:52 AM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

Right, sounds good, sorry for the noise.

andreadb added inline comments.May 18 2018, 9:32 AM
include/llvm/Target/TargetInstrPredicate.td
110 ↗(On Diff #146628)

No problem. In retrospect, I should have created patch 2 of 3 from the start.
To avoid confusion, I have just abandoned D46697 in favor of D47077.

I hope it helps.

Cheers,
Andrea

javed.absar added inline comments.May 20 2018, 9:21 AM
include/llvm/Target/TargetInstrPredicate.td
190 ↗(On Diff #147481)

Hi Andrea.
This example/documentation up to this point was very clear. Thank you for that. But the '#CinstFn' is bit confusing. Maybe you could elaborate this one a bit.

Or may be its just me that's confused.

andreadb added inline comments.May 21 2018, 2:56 AM
include/llvm/Target/TargetInstrPredicate.td
190 ↗(On Diff #147481)

The idea is that MCInstFn and MachineInstrFn are both function names.

MCInstFn is the name of a function that takes as input an MCInst (either by reference or pointer), and returns a bool.
MachineInstrFn is the name of a function that takes as input MachineInstr and returns a bool.

MCInstFn is used by the PredicateExpander when lowering checks on MCInst.
MachineInstrFn is used by the PredicateExpander when lowering checks on MachineInstr.

If the content of lines [189:194] is what confuses you, then I can remove it.

Thanks,
Andrea

andreadb updated this revision to Diff 147771.May 21 2018, 5:42 AM

Addressed review comments.

  • Renamed predicate CheckFunctionCall into CheckFunctionPredicate.
  • Improved the code comment associated with the definition of CheckFunctionPredicate in TargetInstrPredicate.td.

LGTM as an initial NFC(ish) setup stage - does anyone have any further comments or reasons why it shouldn't be committed?

RKSimon accepted this revision.May 25 2018, 7:18 AM

LGTM in conjunction with D47077

This revision is now accepted and ready to land.May 25 2018, 7:18 AM
This revision was automatically updated to reflect the committed changes.