Page MenuHomePhabricator

[mlir][openmp][NFC]Remove unnecessary brackets and rephrase ParallelOp description in mlir definition
ClosedPublic

Authored by Rin on Oct 2 2020, 8:29 AM.

Diff Detail

Event Timeline

Rin created this revision.Oct 2 2020, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 8:29 AM
Rin requested review of this revision.Oct 2 2020, 8:29 AM
Rin updated this revision to Diff 295834.Oct 2 2020, 8:31 AM

Clang-format

Rin added projects: Restricted Project, Restricted Project.
Harbormaster completed remote builds in B73795: Diff 295834.
clementval accepted this revision.Oct 2 2020, 9:01 AM
clementval added a subscriber: clementval.

Can you flag this commit message with [openmp] and maybe [NFC]?

This revision is now accepted and ready to land.Oct 2 2020, 9:01 AM
Rin added a comment.Oct 2 2020, 9:08 AM

Can you flag this commit message with [openmp] and maybe [NFC]?

sure, will do

rriddle added inline comments.Oct 2 2020, 11:17 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
229–232

All of these calls can fail, you have to check the result.

SouraVX added inline comments.Oct 2 2020, 12:51 PM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
270

I can't recall, but I saw somewhere(may LLVM coding style). Not to uses braces for if if there is only one executable statement. Don't have a strong opinion on this, but it would be better if someone can clarify which style to follow.

clementval added inline comments.Oct 2 2020, 1:07 PM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
270

You should remove the braces for trivial if statement. So like this it's fine.

clementval requested changes to this revision.Oct 2 2020, 1:08 PM
clementval added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
229–232

Right. @Rin Can you add the checks?

This revision now requires changes to proceed.Oct 2 2020, 1:08 PM
Rin updated this revision to Diff 296197.Oct 5 2020, 9:09 AM

Address review comments

Rin retitled this revision from [mlir]Remove unnecessary brackets and rephrase ParallelOp description in mlir definition to [mlir][openmp][NFC]Remove unnecessary brackets and rephrase ParallelOp description in mlir definition.Oct 5 2020, 9:09 AM
Rin updated this revision to Diff 296199.Oct 5 2020, 9:11 AM

Clang-format

Rin marked 2 inline comments as done.Oct 5 2020, 9:12 AM
Rin added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
229–232

Sure thing

Harbormaster completed remote builds in B74008: Diff 296199.
Rin updated this revision to Diff 296209.Oct 5 2020, 9:59 AM
Rin marked an inline comment as done.

Address review comments

Rin added a comment.Oct 8 2020, 1:14 AM

@clementval Does this look alright?

clementval accepted this revision.Oct 8 2020, 6:39 AM

If tests pass it's fine for me.

This revision is now accepted and ready to land.Oct 8 2020, 6:39 AM