This is an archive of the discontinued LLVM Phabricator instance.

Add tanh lowering from Standard dialect to NVVM and ROCDL.
ClosedPublic

Authored by dfki-jugr on Jan 27 2020, 6:30 AM.

Details

Summary

The tanh lowering from Standard dialect to NVVM and ROCDL was not working.
The conversion pattern are inserted in the lowering files.
The test cases for the lowerings were added in the test files.

Diff Detail

Event Timeline

dfki-jugr created this revision.Jan 27 2020, 6:30 AM
ftynse accepted this revision.Jan 27 2020, 6:38 AM
ftynse added a subscriber: ftynse.

Please update the commit message from "fixed tanh lowering" to "add tanh lowering". It was not broken (which would mandate a fix), it was never implemented to start with. Good to go otherwise.

This revision is now accepted and ready to land.Jan 27 2020, 6:38 AM

Unit tests: pass. 62198 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dfki-jugr retitled this revision from Fixed tanh lowering from Standard dialect to NVVM and ROCDL. to Add tanh lowering from Standard dialect to NVVM and ROCDL..Jan 27 2020, 7:20 AM
dfki-jugr updated this revision to Diff 240586.Jan 27 2020, 7:45 AM

Fixed clang format issue.

Unit tests: pass. 62198 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

herhut accepted this revision.Jan 27 2020, 11:12 AM

Thank you for adding this!

This revision was automatically updated to reflect the committed changes.

These two tests are failing on Windows like so:

##[error]mlir\test\Conversion\GPUToROCDL\gpu-to-rocdl.mlir(87,12): Error GDC2C77F0: CHECK: expected string not found in input
     1>E:\agent\_work\4\s\mlir\test\Conversion\GPUToROCDL\gpu-to-rocdl.mlir(87,12): error GDC2C77F0: CHECK: expected string not found in input [e:\agent\_work\4\b\check-all.vcxproj]
         
          // CHECK: llvm.func @_ocml_tanh_f32(!llvm.float) -> !llvm.float
         
                    ^
         
         <stdin>:66:2: note: scanning from here
         
          llvm.return
         
          ^
         
         <stdin>:73:2: note: possible intended match here
         
          llvm.func @tanhf(!llvm.float) -> !llvm.float
         
          ^

It looks like the fix is fairly straightforward at least.

We built the project on a Windows machine and were able to reproduce the failed test. We figured out that the tests are right, but the error was caused by a non-deterministic pattern rewriter step. We fixed the issue in another PR: https://reviews.llvm.org/D73713.
This fix should work on all platforms (Windows/Linux).

mlir/test/Conversion/GPUToNVVM/gpu-to-nvvm.mlir
174

@stella.stamenova is the determinism issue resolved by replacing all the new introduced CHECK by CHECK-DAG ?

@rriddle @mehdi_amini this is the original root cause of the benefit twiddling revision.
I can't tell offhand whether all the pieces involved in rewriting and lowering to LLVM are supposed to be deterministic across targets?

CHECK-DAG makes no difference. The output simply does not contain the expected values as written. The test expects to find @_ocml_tanh_f32 (e.g. llvm.func @_ocml_tanh_f32(!llvm.float) -> !llvm.float), but the output is:

module {
  llvm.func @tanh(!llvm.double) -> !llvm.double
  llvm.func @tanhf(!llvm.float) -> !llvm.float
  gpu.module @kernel_module {
    llvm.func @gpu_tanh(%arg0: !llvm.float, %arg1: !llvm.double) {
      %0 = llvm.call @tanhf(%arg0) : (!llvm.float) -> !llvm.float
      %1 = llvm.call @tanh(%arg1) : (!llvm.double) -> !llvm.double
      llvm.return
    }
  }
}