Page MenuHomePhabricator

[MLIR][PDL] Refactor the positions for multi-root patterns.
ClosedPublic

Authored by sfuniak on Dec 20 2021, 9:49 PM.

Details

Summary

When the original version of multi-root patterns was reviewed, several improvements were made to the pdl_interp operations during the review process. Specifically, the "get users of a value at the specified operand index" was split up into "get users" and "compare the users' operands with that value". The iterative execution was also cleaned up to pdl_interp.foreach. However, the positions in the pdl-to-pdl_interp lowering were not similarly refactored. This introduced several problems, including hard-to-detect bugs in the lowering and duplicate evaluation of pdl_interp.get_users.

This diff cleans up the positions. The "upward" OperationPosition was split-out into UsersPosition and ForEachPosition, and the operand comparison was replaced with a simple predicate. In the process, I fixed three bugs:

  1. When multiple roots were had the same connector (i.e., a node that they shared with a subtree at the previously visited root), we would generate a single foreach loop rather than one foreach loop for each such root. The reason for this is that such connectors shared the position. The solution for this is to add root index as an id to the newly introduced ForEachPosition.
  2. Previously, we would use pdl_interp.get_operands indiscriminately, whether or not the operand was variadic. We now correctly detect variadic operands and insert pdl_interp.get_operand when needed.
  3. In certain corner cases, we would trigger the "connector has not been traversed yet" assertion. This was caused by not inserting the values during the upward traversal correctly. This has now been fixed.

Diff Detail

Event Timeline

sfuniak created this revision.Dec 20 2021, 9:49 PM
sfuniak requested review of this revision.Dec 20 2021, 9:49 PM
sfuniak retitled this revision from Refactor the positions for multi-root patterns. to [MLIR][PDL] Refactor the positions for multi-root patterns..Dec 20 2021, 9:49 PM
Mogball accepted this revision.EditedDec 21 2021, 11:07 AM

LGTM, thanks! Just two small things on my end.

mlir/lib/Conversion/PDLToPDLInterp/PredicateTree.cpp
508

Can you expand the description to indicate that the function returns true if the operand at the given index needs to be queried using operand groups if it's variadic itself or follows a variadic operand?

660–662

Use llvm::enumerate if you want the index too.

This revision is now accepted and ready to land.Dec 21 2021, 11:07 AM
sfuniak updated this revision to Diff 395764.Dec 21 2021, 4:02 PM

Incorporated review feedback.

sfuniak updated this revision to Diff 397173.Jan 3 2022, 6:27 PM

Rebased off main.

This revision was landed with ongoing or failed builds.Jan 3 2022, 6:39 PM
This revision was automatically updated to reflect the committed changes.