This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Added Abs, Ceil and Cos to GPU lowering conversion.
ClosedPublic

Authored by dfki-jugr on Jan 9 2020, 2:07 AM.

Diff Detail

Event Timeline

dfki-jugr created this revision.Jan 9 2020, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2020, 2:07 AM
dfki-mako removed a subscriber: dfki-mako.
dfki-mako added a subscriber: dfki-mako.
ftynse added a subscriber: ftynse.Jan 9 2020, 2:12 AM

Looks good to me in general, please see inline.

mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir
48

What's the point of exercising the same operation twice?

pifon2a requested changes to this revision.Jan 9 2020, 2:33 AM
pifon2a added inline comments.
mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir
48

I think this is copied from expf test where I was checking that the symbols are not added twice to the symbol table. I think here it is enough to exercise the op only once.

This revision now requires changes to proceed.Jan 9 2020, 2:33 AM

Unit tests: pass. 61326 tests passed, 0 failed and 737 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dfki-jugr updated this revision to Diff 237010.Jan 9 2020, 4:01 AM

Update clang-format issues.

dfki-jugr updated this revision to Diff 237011.Jan 9 2020, 4:08 AM

Removed unnecessary test cases in test files.

ftynse accepted this revision.Jan 9 2020, 4:57 AM
pifon2a accepted this revision.Jan 9 2020, 5:51 AM

Please, reformat the code and submit.

This revision is now accepted and ready to land.Jan 9 2020, 5:51 AM

When uploading a diff, can you make sure to diff against HEAD instead of the previous commit? I couldn't see any of your changes and had to manually look through the diff history.

mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp
715 ↗(On Diff #236981)

Can we merge all of these instead?

addIllegalOp<..., ..., ..., ...>();

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp
66 ↗(On Diff #236981)

Same here.

dfki-jugr updated this revision to Diff 237592.Jan 13 2020, 1:22 AM

Merged template arguments in AddIllegalOP.

dfki-jugr marked 2 inline comments as done.Jan 13 2020, 1:23 AM
pifon2a accepted this revision.Jan 13 2020, 1:42 AM

Do you guys have commit access or should I land this for you?

Do you guys have commit access or should I land this for you?

I have commit access, I can land it.

This revision was automatically updated to reflect the committed changes.