search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.97k stars 11.94k forks source link

[AArch64] machine combiner converts independent fadds into dependent fadds #37137

Open rotateright opened 6 years ago

rotateright commented 6 years ago
Bugzilla Link 37789
Version trunk
OS All
CC @DMG862,@efriedma-quic,@TNorthover

Extended Description

Copied from: test/CodeGen/AArch64/fadd-combines.ll

; DAGCombiner transforms this into: (x + 59.0) + (x + 17.0). ; The machine combiner transforms this into a chain of 3 dependent adds: ; ((x + 59.0) + 17.0) + x

define float @​fadd_const_multiuse_attr(float %x) #​0 { ; CHECK-LABEL: fadd_const_multiuse_attr: ; CHECK: // %bb.0: ; CHECK-NEXT: adrp x9, .LCPI8_1 ; CHECK-NEXT: adrp x8, .LCPI8_0 ; CHECK-NEXT: ldr s1, [x9, :lo12:.LCPI8_1] ; CHECK-NEXT: ldr s2, [x8, :lo12:.LCPI8_0] ; CHECK-NEXT: fadd s1, s0, s1 ; CHECK-NEXT: fadd s1, s2, s1 ; CHECK-NEXT: fadd s0, s0, s1 ; CHECK-NEXT: ret %a1 = fadd float %x, 42.0 %a2 = fadd float %a1, 17.0 %a3 = fadd float %a1, %a2 ret float %a3 }

That's with no CPU model specified. When I tried setting that to various existing models, the transform doesn't happen.

I don't see why we would do this transform under any circumstances, but I might be missing some AArch subtlety.

efriedma-quic commented 6 years ago

MachineCombiner reassociation is supposed to try to reduce the length of the critical path; not sure what it's doing in your testcase.

rotateright commented 6 years ago

cc'ing some more knowledgeable AArch people.

Also - forgot to include the necessary function attributes in the paste. We're allowed to reassociate the FP math because:

attributes #​0 = { "unsafe-fp-math"="true" }