This is an archive of the discontinued LLVM Phabricator instance.

[MLIR,OpenMP] Lowering of parallel operation: proc_bind clause 2/n
ClosedPublic

Authored by kiranchandramohan on Jul 22 2020, 10:44 AM.

Details

Summary

This simple patch adds the translation of the proc_bind clause in a parallel operation. The string representation of the proc_bind operand is retrieved from the operation and converted to the enum value expected by the OpenMP IRBuilder.

A new function is added to get the enum value of a proc_bind clause's string representation in a new file (llvm/lib/Frontend/OpenMP/OMPConstants.cpp).

Diff Detail

Event Timeline

jdoerfert added inline comments.Jul 22 2020, 7:16 PM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
82

Documentation please.

llvm/lib/Frontend/OpenMP/OMPConstants.cpp
29 ↗(On Diff #279880)

We had a file like this before, I think. We removed it because the content was generated. Maybe this is the time to move the proc bind enum to tablegen?

DavidTruby added inline comments.Jul 31 2020, 6:28 AM
llvm/lib/Frontend/OpenMP/OMPConstants.cpp
29 ↗(On Diff #279880)

Personally I think moving proc-bind to tablegen should be a separate patch done either before or after this one, rather than as part of this patch. I agree that it should be done though!

jdoerfert added inline comments.Jul 31 2020, 3:28 PM
llvm/lib/Frontend/OpenMP/OMPConstants.cpp
29 ↗(On Diff #279880)

Two patches are even better. Introducing the OMPConstants again to remove it right away seems a bit odd though ;)

Addressing @jdoerfert's review comment.

The values that can be specified for the proc_bind clause are specified in the OMP.td tablegen file in the llvm/Frontend/OpenMP directory. From this single source of truth enumeration for proc_bind is generated in llvm and mlir (used in specification of the parallel Operation in the OpenMP dialect). A function to return the enum value from the string representation is also generated.

Added few comments but generally it looks good. Can you maybe complete the test directive1.td or directive2.td with a ClauseVal so we can check the generated code in the test suite.

llvm/include/llvm/Frontend/Directive/DirectiveBase.td
87

Thanks for the fix. Can be push without review.

llvm/utils/TableGen/DirectiveEmitter.cpp
212

What happens if we omit the prefix OMPC_ when we define the ClauseVal ... name will be truncated right? I guess you could define the ClauseVal without the prefix in the .td file and drop the string manipulation here. It will probably be safer.

218

Do you assume names have no spaces? Otherwise you should do the same trick than for the directive and clauses to add a _ for the enum name.

229

Do you want enumName twice in the name of your function?

349

Can be push without review.

396

Same comment here about the prefix.

mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
9

Is this new tablegen specific to OpenMP or do you think we can use it for other directive language (e.g. OpenACC). No need to change this now, we can do when we see a need. Just asking your opinion.

clementval added inline comments.Aug 6 2020, 12:38 PM
llvm/utils/TableGen/DirectiveEmitter.cpp
212

The clang-tidy rule for TableGen code force a capital latter for variable. So enumName should be EnumName. Can you update this and other places if needed?

jdoerfert added inline comments.Aug 6 2020, 1:07 PM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
80

We can delete OMP_PROC_BIND_KIND from OpenMPKinds.def now, right?

clementval requested changes to this revision.Aug 6 2020, 7:02 PM
clementval added inline comments.
llvm/utils/TableGen/DirectiveEmitter.cpp
207

We now have small wrapper to access record information -> https://github.com/llvm/llvm-project/blob/master/llvm/utils/TableGen/DirectiveEmitter.cpp#L135

Can you use them and add one for ClauseVal?

This revision now requires changes to proceed.Aug 6 2020, 7:02 PM

Some replies before addressing comments.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
80

Good Catch.

There is a usage in clang/lib/Basic/OpenMPKinds.cpp. I will try to remove that also.

llvm/utils/TableGen/DirectiveEmitter.cpp
212

The OMPC_ prefix is there for the clauses. Have not used it for the ClauseVals. In this case, we have the following definition for the ProcBind clause. The new code is generating the name of the enumeration "ProcBindKind". Because the prefix is also a setting in the tablegen file I felt it is safe to use. Is that OK?

def OMPC_ProcBind : Clause<"proc_bind"> {

let clangClass = "OMPProcBindClause";
let allowedClauseValues = [
  OMP_PROC_BIND_master,
  OMP_PROC_BIND_close,
  OMP_PROC_BIND_spread,
  OMP_PROC_BIND_default,
  OMP_PROC_BIND_unknown
];

}

212

will change.

218

Yes, that is the assumption.
BTW, Is that what the formattedName function does?

Will change.

229

Yes, the first is the return type (enum) and the second is the name of the function.

ProcBind getProcBind(StringRef)

396

Have provided an explanation above.

mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
9

No, nothing specific to OpenMP now. Can rename to Directive.

clementval added inline comments.Aug 7 2020, 6:28 AM
llvm/utils/TableGen/DirectiveEmitter.cpp
212

The prefix in the DirectiveLanguage record doesn't apply for the clause or directive records found in the tablegen file. It is just used for the code generation. It is a coincidence that the records use the same prefix in their name. At least their is no enforcement that the records follow the given prefix for directive or clause. So it might be risky.

218

Yes the function does that.

229

Fine! I missed the space :-)

396

Same answer as above.

mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
9

Up to you. As I said, this can be done when and if we see a need to share this with OpenACC for example.

kiranchandramohan marked 15 inline comments as done.

Addressing @clementval's comments.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
80

Removing the usage in Clang would require making changes for all Clauses having enums. Can do that as a separate patch. Is that OK?

82

Is this a doxygen documentation you are asking for?

llvm/utils/TableGen/DirectiveEmitter.cpp
207

Done and moved all wrappers to llvm/include/llvm/TableGen/DirectiveEmitter.h so that they can be used in MLIR also.

212

Now using a field in the Clause Record to get the name of enum class.
let enumClauseValue = "ProcBindKind";

218

I am using getFormattedName in some places now. But not for this specific instance. Here i am using the name of the record as the enumeration value. I did not want to create another field just for the name of the enumeration value. Specifically, i am using OMP_PROC_BIND_master etc as the enum value name. These will not have spaces in them.

let clangClass = "OMPProcBindClause";
let allowedClauseValues = [

OMP_PROC_BIND_master,
OMP_PROC_BIND_close,
OMP_PROC_BIND_spread,
OMP_PROC_BIND_default,
OMP_PROC_BIND_unknown

];

mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
9

OK. not making changes now. We can rename when one more user comes up.

jdoerfert added inline comments.Aug 11 2020, 6:31 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
80

Yes, separate patch is fine.

82

yes, but the functions are gone already I suppose. All functions and members and classes in lib/Frontend/OpenMP should have doxygen comments.

@clementval I guess I need to rebase with your recent changes and update the tests.

@kiranchandramohan Yeah sorry about that. Hopefully should not be too complicated.

clementval accepted this revision.Aug 11 2020, 11:40 AM

LGTM. Thanks for making the changes.

llvm/utils/TableGen/DirectiveEmitter.cpp
207

Good move!

218

Sounds good!

mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
9

Fine.

This revision is now accepted and ready to land.Aug 11 2020, 11:40 AM

Adding a testcase and rebasing on top of a recent commit in this area from Valentin.

clementval accepted this revision.Aug 11 2020, 5:23 PM

Still LGTM

mehdi_amini added inline comments.Aug 12 2020, 2:17 AM
mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
28

Can you document this?
I rather not reverse engineer the code to understand the contract on the input and the generated code.

mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
28

Sure. I will document this in a patch later today.
Assuming some comments here is what is expected. Or should it be as part of the Operation or Dialect definition?

Thanks for fixing the documentation dependency issue.

mehdi_amini added inline comments.Aug 12 2020, 9:00 AM
mlir/tools/mlir-tblgen/OpenMPCommonGen.cpp
28

Yes just commenting here what is this TableGen backend about for future readers.