This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Pretty printer and parser for omp.wsloop
ClosedPublic

Authored by kiranchandramohan on Nov 30 2020, 8:11 AM.

Details

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
kiranchandramohan requested review of this revision.Nov 30 2020, 8:12 AM
ftynse added a comment.Dec 7 2020, 4:43 AM

I find the linear(%data_var : memref<i32> -> %linear_var) a bit confusing, we are using an equivalent of linear(%linear_var = %data_var : memref<i32>) in SCF and GPU dialects, and generally the %within_region = %outside_region : type pattern applies even more operations. -> seems to be predominantly used for results, which makes it confusing for me here.

kiranchandramohan edited the summary of this revision. (Show Details)

Fixed failure in SCFToOpenMP.
Converted "->" to "=" for linear clause.

I find the linear(%data_var : memref<i32> -> %linear_var) a bit confusing, we are using an equivalent of linear(%linear_var = %data_var : memref<i32>) in SCF and GPU dialects, and generally the %within_region = %outside_region : type pattern applies even more operations. -> seems to be predominantly used for results, which makes it confusing for me here.

I have used "=" now.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
457–461

If we want to support loops with different integer types (like index, i32, i64 etc), can we have a syntax like the following?

omp.wsloop (%i) = (%lb) to (%ub) step (%c)

or should it be like the following?

omp.wsloop (%i : i32) = (%lb : i32) to (%ub : i32) step (%c : i32)

And will the code given above for resolveOperands work by trying to parse with different types?

kiranchandramohan retitled this revision from [MLIR][OpenMP][WIP] Pretty printer and parser for omp.wsloop to [MLIR][OpenMP] Pretty printer and parser for omp.wsloop.Mar 10 2021, 3:19 PM
kiranchandramohan edited the summary of this revision. (Show Details)

Added multi-block tests.

ftynse requested changes to this revision.Mar 12 2021, 4:20 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
197

Nit: 80cols plz

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
378

Please add a comment with pseudo-BNF of what this parses.

437

s/omp.do/omp.wsloop

443–444

There's no difference between integer types and llvm integer types anymore

457–461

If the op only accepts operands of _identical_ types (which sounds sensible here), it suffices to list the type once, something like

omp.wsloop (%i) : (i32) = (%lb) to %(ub) step (%c) or omp.wsloop (%i : i32) = (%lb) to %(ub) step (%c).

The try-and-fail resolution won't work. The value will be null in case of type mismatch https://github.com/llvm/llvm-project/blob/f6524b4ada823a9766f7cbdfc16e052971e005f6/mlir/lib/Parser/Parser.cpp#L541. Even if it did, it would complain about every mismatching type. You need to parse a specific type instead and use it in resolution. I also suggest _not_ to check which type it is and delegate it to the verifier, just using AnyTypeOf<[Index, I32, I64]> for arguments in ODS combined with AllTypesMatch trait should suffice.

498

The code above supports parsing multiple lbs, ubs and steps, but this hardcodes 1 as their number.

626

Nit: we don't need an explicit number of stack values for SmallVector anymore, unless there is a strong reason to put 4 specifically

633–637

This likely won't work. In any case, at this point you should already know the actual types of region entry arguments - they are the same as lbs/ubs

649

Nit: we prefer early return:

if (vars.empty())
  return;
// code
651

Nit: don't call .size() on each iteration

651–654

llvm::interleaveComma is your friend here

674

Please fix the linter

This revision now requires changes to proceed.Mar 12 2021, 4:20 AM
kiranchandramohan marked 12 inline comments as done.
kiranchandramohan edited the summary of this revision. (Show Details)

Address review comments.
Updated syntax to specify the type along with the index.
Using BNF for parsing function documentation
Minor fixes, using llvm::interleaveComma.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
457–461

Thanks for your help here.

I have used the following style.

omp.wsloop (%i) : i32 = (%lb) to %(ub) step (%c)
651–654

Done.
For the linearVars case below i have not used since it refers to two containers linearVars, op.linear_step_vars().

ftynse accepted this revision.Mar 15 2021, 1:43 AM
ftynse added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
410

Nit: this reads like one can just omit the =, use ('=' ssa-id-and-type)? for a group

411

ultra-nit: delimit literals with backticks

509

Please drop llvm:: from Optional and StringRef, they are re-exported into the mlir namespace.

535

Nit: capitalize the first word and terminate sentence with a period.

728

Why do we need this trivial loop?

This revision is now accepted and ready to land.Mar 15 2021, 1:43 AM
kiranchandramohan marked 5 inline comments as done.

Addressed review comments.