This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Removed TanHOp lowering from ConvertStandardToLLVM since there is no reasonable TanH representation in LLVM.
ClosedPublic

Authored by dfki-mako on Mar 3 2020, 2:10 AM.

Details

Summary

The current ConvertStandardToLLVM phase lowers the standard TanHOp to function calls to external tanh symbols. However, this leads to misunderstandings since these external symbols are not defined anywhere. This commit removes the TanHOp lowering functionality from ConvertStandardToLLVM, adapts the LowerGpuOpsToNVVMOps and LowerGpuOpsToROCDLOps passes and adjusts the affected test cases.

Diff Detail

Event Timeline

dfki-mako created this revision.Mar 3 2020, 2:10 AM
pifon2a accepted this revision.Mar 3 2020, 5:49 AM
This revision is now accepted and ready to land.Mar 3 2020, 5:49 AM
rriddle accepted this revision.Mar 3 2020, 9:19 AM
herhut accepted this revision.Mar 12 2020, 7:19 AM

Can this land or has it already?

mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp
689

Could you also remove the filterIllegalLLVMIntrinsics if it is now unused? It was a hack to unbreak the windows build and we should remove it to prevent others from starting to use it.

dfki-mako updated this revision to Diff 250748.Mar 17 2020, 6:23 AM

Removed unused filterIllegalLLVMIntrinsics function.

herhut accepted this revision.Mar 19 2020, 8:37 AM
This revision was automatically updated to reflect the committed changes.
dfki-mako marked an inline comment as done.

Hello! I am confused about the reasoning why this functionality was removed - we certainly need the it. I did coded up pretty much the same change and someone (thanks Laub, Frank) pointed out this diff. What is the general strategy/approach to lower operations that are not intrinsics in the LLVM.

Herald added a project: Restricted Project. · View Herald Transcript

@llitchev The Tanh approximation is definitely needed, but the main problem of this very PR was that StandardToLLVM was a wrong place for this code to be in. It might be possible to stay within Standard dialect and make it a transform, i.e. lower std.tanh to std ops and only then lower to LLVM using the StandardToLLVM pass.
The could would live then in the same directory with https://github.com/llvm/llvm-project/blob/master/mlir/lib/Dialect/StandardOps/Transforms/ExpandAtomic.cpp.

@ftynse: Alex, do we already have any other math func approximation like for tanh submitted?

@pifon2a Thanks! That makes sense.

ftynse added a comment.Jun 5 2020, 2:40 AM

@ftynse: Alex, do we already have any other math func approximation like for tanh submitted?

I don't think we do. But Standard/Transforms would be the right place for them to live IMO.

ftynse added a comment.Jun 5 2020, 2:43 AM

@llitchev There are two guiding principles for this commit:

  • The LLVM dialect must not have anything that is not present in LLVM IR
  • Separation of concerns makes conversions simpler: if we can do the tanh expansion on the standard dialect (without involving type conversions necessary for std->llvm), we absolutely should for the sake of maintainability.
flaub added a subscriber: flaub.Jun 9 2020, 1:19 PM