Page MenuHomePhabricator

[gicombiner] Hoist pure C++ combine into the tablegen definition
ClosedPublic

Authored by dsanders on Oct 3 2019, 2:41 PM.

Details

Summary

This is just moving the existing C++ code around and will be NFC w.r.t
AArch64. Renamed 'CombineBr' to something more descriptive
('ElideByByInvertingCond') at the same time.

The remaining combines in AArch64PreLegalizeCombiner require features that
aren't implemented at this point and will be hoisted as they are added.

Depends on D68424

Diff Detail

Event Timeline

dsanders created this revision.Oct 3 2019, 2:41 PM
volkan added inline comments.Wed, Oct 16, 1:22 PM
llvm/utils/TableGen/GICombinerEmitter.cpp
270

Rules must have a matcher, right? If so, why do we need 1 && here?

283

Don't we need to get the Matcher here as we do that for Applyer below?

dsanders marked 2 inline comments as done.Wed, Oct 16, 3:29 PM
dsanders added inline comments.
llvm/utils/TableGen/GICombinerEmitter.cpp
270

Yes but they don't necessarily use the trap-door into the C++ block (eventually none will and I'll remove it). At this point in the patch series, that trap-door is the only thing the matcher can use so they all use it but in subsequent patches there will be enough features to move the code into DAG matchers and/or custom predicates. Even later, predicates that don't get tested as part of the decision tree will be resolved in this if-statement.

For why the 1 && is in the output, it's just to keep the code simple. Without it, I'd have to special case the case where there's no checks but with it I can just have each check emit && foo()

283

At the moment, the matcher can only contain a code block and this is essentially a trap-door that allows me to fix up the generated code and keep things working while the features are going in. I could rename getCode() to getMatcherFixupCode() if that helps.

dsanders updated this revision to Diff 225325.Wed, Oct 16, 3:51 PM

Rename the variable holding the fixup code block

volkan accepted this revision.Wed, Oct 16, 3:59 PM

Thanks for explaining, LGTM.

llvm/utils/TableGen/GICombinerEmitter.cpp
283

I think that would make it easier to understand.

This revision is now accepted and ready to land.Wed, Oct 16, 3:59 PM
This revision was automatically updated to reflect the committed changes.