Page MenuHomePhabricator

[MLIR][Math] Emit llvm.func when converting to libm
Needs ReviewPublic

Authored by chelini on Jun 3 2022, 7:29 AM.

Details

Summary

Currently, conversion to libm does not play nicely with
convert-func-to-llvm='emit-c-wrappers=1'. Conversion from func to LLVM emits a
'_mlir_ciface_*' call which is not the intended behaviour here. Emit an
llvm.func instead of a func.func when converting to libm.

Example:

llvm.func @tanh_caller(%float: f32) -> (f32) {
 %float_result = math.tanh %float : f32
 llvm.return %float_result : f32
}

Will convert to:

llvm.func @tanhf(%arg0: f32) -> f32 attributes {sym_visibility = "private"} {
  %0 = llvm.call @_mlir_ciface_tanhf(%arg0) : (f32) -> f32
  llvm.return %0 : f32
}

llvm.func @_mlir_ciface_tanhf(f32) -> f32 attributes {sym_visibility = "private"}

llvm.func @tanh_caller(%arg0: f32) -> f32 {
  %0 = llvm.call @tanhf(%arg0) : (f32) -> f32
  llvm.return %0 : f32
}

This is not what you want in this situation.

Diff Detail

Event Timeline

chelini created this revision.Jun 3 2022, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 7:29 AM
chelini requested review of this revision.Jun 3 2022, 7:29 AM
chelini added reviewers: ftynse, wsmoses.EditedJun 3 2022, 7:29 AM

I noticed that this exact behavior applies when converting complex to libm. I will follow up with a patch for complex if this is the right direction.

ftynse added a comment.Jun 7 2022, 6:25 AM

This feels a bit like premature lowering. Do you have a flow that relies on the emit-c-wrappers option? We have a better alternative -- attach the llvm.emit_c_interface attributes to functions that need the interface -- that leaves most function alone. Maybe we should just remove the option instead and only have the attribute, anyone who wants blanket wrapper emission can just add a pass that attaches the attribute to all funcs.

This feels a bit like premature lowering. Do you have a flow that relies on the emit-c-wrappers option? We have a better alternative -- attach the llvm.emit_c_interface attributes to functions that need the interface -- that leaves most function alone. Maybe we should just remove the option instead and only have the attribute, anyone who wants blanket wrapper emission can just add a pass that attaches the attribute to all funcs.

You are right; the flow I have now uses only the 'emit-c-wrapper' option, and I am hitting the same problem Nicolas mentioned in the revision https://reviews.llvm.org/D77314 about undefined symbols. It makes sense to drop the option if we have a better alternative.