[Clang] `-mlink-builtin-bitcode` may change FP32 denormal mode of linked module

[Maetveis]

Description

-mlink-builtin-bitcode may change the FP32 denormal mode of linked module if not explicitly specified.

Reproducer

// a.ll
declare float @add(float, float)

define float @f() #0 {
    ; The constant 0x37F0000000000000 is a denormal number in FP32
    ; 2 x F32(0x37F0000000000000) = F32(0x380000000000000) which is still denormal
    ; This is the expected result. With (incorrect) flushing of denormals we will get +0.
    %1 = call float @add(float 0x37F0000000000000, float 0x37F0000000000000)
    ret float %1
}

attributes #0 = { "denormal-fp-math"="preserve-sign,preserve-sign" }

// bc.ll
define float @add(float %a, float %b) {
    %1 = fadd float %a, %b
    ret float %1
}

Compile a.ll and link in the bitcode from bc.bc.

> llvm-as bc.ll
> clang -cc1 -fdenormal-fp-math=preserve-sign,preserve-sign -mlink-builtin-bitcode bc.bc -emit-llvm -o - a.ll

Observed behaviour

The attributes on @add have been changed to flush denormals (for F32 only):

; (...)
define internal float @add(float %a, float %b) #1 {
; (...)
; attributes #1 = { convergent "denormal-fp-math-f32"="preserve-sign,preserve-sign" ... }

This changes the behaviour of the program, for example turning on optimizations produces:

> clang -cc1 -O1 -fdenormal-fp-math=preserve-sign,preserve-sign -mlink-builtin-bitcode bc.bc -emit-llvm -o - a.ll

; (...)
define noundef float @f() local_unnamed_addr #0 {
  ret float 0.000000e+00
}

Expected behaviour

@add should have no denormal-fp-math(-f32) attributes added to it, this corresponds to the default mode as specified by IEEE.

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Mészáros Gergely (Maetveis)

# Description

-mlink-builtin-bitcode may change the FP32 denormal mode of linked module if not explicitly specified.

Reproducer

// a.ll
declare float @<!-- -->add(float, float)

define float @<!-- -->f() #<!-- -->0 {
    ; The constant 0x37F0000000000000 is a denormal number in FP32
    ; 2 x F32(0x37F0000000000000) = F32(0x380000000000000) which is still denormal
    ; This is the expected result. With (incorrect) flushing of denormals we will get +0.
    %1 = call float @<!-- -->add(float 0x37F0000000000000, float 0x37F0000000000000)
    ret float %1
}

attributes #<!-- -->0 = { "denormal-fp-math"="preserve-sign,preserve-sign" }

// bc.ll
define float @<!-- -->add(float %a, float %b) {
    %1 = fadd float %a, %b
    ret float %1
}

Compile a.ll and link in the bitcode from bc.bc.

> llvm-as bc.ll
> clang -cc1 -fdenormal-fp-math=preserve-sign,preserve-sign -mlink-builtin-bitcode bc.bc -emit-llvm -o - a.ll

Observed behaviour

The attributes on @<!-- -->add have been changed to flush denormals (for F32 only):

; (...)
define internal float @<!-- -->add(float %a, float %b) #<!-- -->1 {
; (...)
; attributes #<!-- -->1 = { convergent "denormal-fp-math-f32"="preserve-sign,preserve-sign" ... }

This changes the behaviour of the program, for example turning on optimizations produces:

> clang -cc1 -O1 -fdenormal-fp-math=preserve-sign,preserve-sign -mlink-builtin-bitcode bc.bc -emit-llvm -o - a.ll

; (...)
define noundef float @<!-- -->f() local_unnamed_addr #<!-- -->0 {
  ret float 0.000000e+00
}

Expected behaviour

@<!-- -->add should have no denormal-fp-math(-f32) attributes added to it, this corresponds to the default mode as specified by IEEE.

[Maetveis]

Is this actually by intention? clang/test/CodeGenCUDA/propagate-attributes.cu:80 looks like its explicitly testing for this behavior.

@arsenm @LewisCrawford @Artem-B

In an ideal world libdevice would just be built with -fdenormal-fp-math-f32=dynamic, but even if we can't have that (or not for older version of the cuda toolkit still supported) this workaround should be reduced in scope IMO.
For example if in the description bc.ll would have attributes #0 = { "denormal-fp-math"="preserve-sign,preserve-sign" } (full flush-to-zero), and the compilation of a.ll would have-fdenormal-fp-math=ieee we would still override the fp32 mode from bc.ll. Which feels wrong.

I wonder if adding a specific analog of -mlink-builtin-bitcode just for CUDA libdevice or looking at !nvvmir.version to activate workarounds like this would be worthwile.

[arsenm]

It's not only intentional, it's the entire point of using -mlink-builtin-bitcode. The usage model of rocm-device-libs/libclc/libdevice is all deeply broken though. We really shouldn't be absorbing bitcode this way in the first place

[Maetveis]

It's not only intentional, it's the entire point of using -mlink-builtin-bitcode. The usage model of rocm-device-libs/libclc/libdevice is all deeply broken though. We really shouldn't be absorbing bitcode this way in the first place

Well..., that's fun. I might just as well close this issue, then. I came across when debugging an inlining problem with DPC++'s libsycl which is based on libclc. There's no true motivation why I'd wanting to build that with non default/dynamic denormal mode though.

I was vaguely aware of the past discussions around -mlink-builtin-bitcode being a giant hack, what would be the correct way to do this, (thin-)LTO? What's stopping us from doing it like that?

[arsenm]

There's no true motivation why I'd wanting to build that with non default/dynamic denormal mode though.

For the purposes of the denormal mode attributes, the correct thing to do is force the bitcode to use the dynamic mode. That downgrades the -mlink-builtin-bitcode treatment of it to an optimization. We then can do IPO optimizations to propagate concrete denormal modes into callees (Attributor has support for this, but it's not on by default. We should port the same to FunctionAttrs. I was working on this about 2 years ago but that's the current state on this). So yes, in principle we should not be overriding denormal-fp-math* but historically was more necessary

I was vaguely aware of the past discussions around -mlink-builtin-bitcode being a giant hack, what would be the correct way to do this, (thin-)LTO? What's stopping us from doing it like that?

It's a long explanation and not all the problems have definitive solutions; I'm hoping to give a presentation at the GPU workshop at the upcoming conference about this

[Artem-B]

-mlink-builtin-bitcode being a giant hack,

The original intended purpose was "good enough to link in NVIDIA's libdevice bitcode". It does have unintended consequences and sharp corners, and it was never intended to replace a general purpose linker that handles everything properly. So, yes, "hack" is an adequate description.