Page MenuHomePhabricator

[mlir][ArmSVE] Add basic mask generation operation
ClosedPublic

Authored by jsetoain on Jun 1 2021, 10:56 AM.

Details

Summary

These arm_sve.cmp functions are needed to generate scalable vector
masks as long as scalable vectors are not part of the standard types.
Once in standard, these can be removed and std.cmp can be used
instead.

Diff Detail

Event Timeline

jsetoain created this revision.Jun 1 2021, 10:56 AM
Matt added a subscriber: Matt.Jun 1 2021, 11:45 AM
Matt added inline comments.Jun 1 2021, 11:50 AM
mlir/include/mlir/Dialect/ArmSVE/ArmSVE.td
446

(Likely) Minor typo: "unordered not equal" (dropping "or").

Matt added inline comments.Jun 1 2021, 11:55 AM
mlir/include/mlir/Dialect/ArmSVE/ArmSVE.td
533

Minor edit: Use integer comparison operation cmpi (instead of cmpf) in the example for cmpi.

jsetoain updated this revision to Diff 349189.Jun 2 2021, 1:10 AM

Amended documentation.
Fixed bug in parseCmpOp (assuming a vector must be LLVMFixed or LLVMScalable)

jsetoain marked 2 inline comments as done.Jun 2 2021, 1:10 AM
jsetoain updated this revision to Diff 349209.Jun 2 2021, 2:48 AM

Added missing empty line

jsetoain updated this revision to Diff 349224.Jun 2 2021, 3:54 AM

Added a slightly more interesting test

jsetoain published this revision for review.Jun 2 2021, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 4:08 AM
ftynse added inline comments.Jun 4 2021, 4:25 AM
mlir/include/mlir/Dialect/ArmSVE/ArmSVE.td
406–432

This will create name clashes should you ever need to include ArmSVE.td and StandardOps/Ops.td in the same file, e.g. for DRR patterns. Have you considered exposing the predicates from StandardOps/Ops.td to StandardOps/StandardOpBase.td and reusing them instead of copying?

jsetoain updated this revision to Diff 349914.Jun 4 2021, 10:30 AM

Moved Cmp[F|I]Predicate from Ops.td to StandardOpsBase.td and reuse them in ArmSVE

jsetoain marked an inline comment as done.Jun 4 2021, 10:32 AM
jsetoain added inline comments.
mlir/include/mlir/Dialect/ArmSVE/ArmSVE.td
406–432

Something like this?

jsetoain marked an inline comment as done.Jun 4 2021, 12:07 PM
ftynse accepted this revision.Jun 7 2021, 1:24 AM
This revision is now accepted and ready to land.Jun 7 2021, 1:24 AM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Jun 8 2021, 1:32 AM
awarzynski added a subscriber: awarzynski.EditedJun 8 2021, 1:50 AM

Hi @jsetoain.

Thank you for working on this! Sadly, this change is causing multiple BuildBot workers to fail:

I suspect that this change was only tested with static lib builds. Could you please test with BUILD_SHARED_LIBS set to On? Thank you!

*EDIT*
Apologies, I've only noticed your comment *after* posting this.

Sorry it took me so long to revert, and thank you for the instructions!

jsetoain updated this revision to Diff 350567.Jun 8 2021, 3:55 AM

Fixed shared library dependency

This revision was automatically updated to reflect the committed changes.