[AArch64] OOptimize floating point materialization

[zatrazz]

Summary

This patch changes isFPImmLegal to return if the value can be enconded
as the immediate operand of a logical instruction besides checking if
for immediate field for fmov.

This optimizes some floating point materization, inclusive values
used on isinf lowering.

[rengolin]

Sounds good. Please add simple tests for +inf and -inf on both double and float. Perhaps also float to double and vice versa from the IR constant (like the changed test shows), too. Thanks!

[zatrazz]

Updated patch with additional tests. I added tests for __builtin_isinf expanded builtin for float, double, and long double (the latter still requires loading the constant).

[rengolin]

LGTM, thanks!

[efriedma]

Is there some reason we're checking for specific floating-point constants here, as opposed to just calling AArch64_AM::isLogicalImmediate or AArch64_AM::isAnyMOVWMovAlias?

lib/Target/AArch64/AArch64ISelLowering.cpp

5429

This comment is wrong.

[zatrazz]

lib/Target/AArch64/AArch64ISelLowering.cpp

5429

What about:

We can materialize #0.0 as fmov $Rd, XZR and #INF as orr $Rx, XZR, CTE
plus fmov $Rd for 64-bit and 32-bit cases.

?

[zatrazz]

In D57044#1367943, @efriedma wrote:

Is there some reason we're checking for specific floating-point constants here, as opposed to just calling AArch64_AM::isLogicalImmediate or AArch64_AM::isAnyMOVWMovAlias?

I am not sure, the DAGCombiner code seems to check on some points if the operation is legal and other if constant can be materialized without load (DAGCombiner::convertSelectOfFPConstantsToLoadOffset).

[efriedma]

It would be a one-line change to make the AArch64 target treat all FP immediates as legal (this was implemented in https://reviews.llvm.org/rL223941 , but only enabled for the large code model).

In terms of what's actually profitable, it's not entirely obvious. If you look at timings on a Cortex-A57, the cost is actually exactly the same for mov+fmov vs. adrp+ldr; the mov+fmov sequence is always better because of the reduced cache pressure. And actually, the timings are still the same if you consider movw+movk+fmov vs. adrp+ldr (it's one instruction longer, but the movw+movk is fused). For f64, though, it's not obviously worthwhile to emit a five-instruction sequence vs. a two instruction constant-pool load. Some quick testing with gcc shows it emits mov+movk, but not longer sequences.

We could add a separate target hook for DAGCombiner::SimplifySelectCC, maybe, to distinguish between illegal immediates and immediates which are legal-but-expensive. Probably worth revising that code anyway; it currently doesn't consider the possibility of transforming to an integer SELECT_CC, then BITCASTing the result to a float.

We could also add code to use the SIMD MOVI for floating-point immediates in certain cases (such immediates are probably not that common, but you can make some interesting numbers like 32.f and -0.f).

In any case, it doesn't really make sense to special-case infinity, specifically.

[zatrazz]

Updated patch from previous comments. I added an extra check using AArch64_AM::isLogicalImmediate for constants that can be materialized as the immediate operand of a logical instruction. We still need to handle the cases for some 8-bit immediates on fmov using getFPXXImm and positive 0.0.

[efriedma]

lib/Target/AArch64/AArch64ISelLowering.cpp

5446

"operanf"

5451

I have no idea what isLogicalImmediate does if the RegSize is 16.

[zatrazz]

lib/Target/AArch64/AArch64ISelLowering.cpp

5446

Fixed locally.

5451

My understanding it should be safe since fmov transfer for float16 will be done from 32 bit register.

[efriedma]

lib/Target/AArch64/AArch64ISelLowering.cpp

5451

The proposed transform is fine for f16; I'm more concerned that isLogicalImmediate() itself won't return the right result, since the actual underlying operation is a 32-bit orr.

Actually, thinking about it a bit more, we might as well just mark ISD::ConstantFP Legal for f16, since it can always be done in two instructions.

On a related note, not sure the ISel patterns you need for f16 are implemented.

[zatrazz]

Patch updated based on previous comments. The isAnyMOVWMovAlias
can catch more cases where we can materialize the floating-point constant
than isLogicalImmediate (128 .0 for instance). The positive zero still need to
be handled as specific case because the resulting fmov will use the zero
register instead of an immediate. The isAnyMOVWMovAlias path is not use
for fp16 (I will need to check further if is safe for all cases).

As a side note there still some cases where we can materialize the a fp-constant
without loads that current patch does not handle. The negative zero, for instance,
can be materialized as 'movi v0.2s, 0x80, lsl 24'. I will check how we can handle
cases like this.

[zatrazz]

Ping.

[efriedma]

Do we have test coverage for the various f16 cases somewhere? I'd like to make sure we don't crash or miscompile for f16 inf.

lib/Target/AArch64/AArch64ISelLowering.cpp

5446

Maybe clarify precisely what sequence you expect will be emitted here?

5456

"dbgs() << Imm" should work for APInt, I think.

[zatrazz]

Updated patch based on previous comment:

• Added a comment about isAnyMOVWMovAlias check intention.
• Added _Float16 isinf check.

Also, unfortunately dbgs() does not have APFloat support yet.

[efriedma]

LGTM with one nit.

lib/Target/AArch64/AArch64ISelLowering.cpp

5447

MOVZ, MOVN, or ORR.

[evandro]

lib/Target/AArch64/AArch64ISelLowering.cpp

5448

Since fmov h0, w0 is legal, shouldn't MVT::f16 be added here too?

[efriedma]

lib/Target/AArch64/AArch64ISelLowering.cpp

5448

Yes, it's possible, but I'm fine handling it in a followup. We currently don't have an isel pattern to generate that fmov.

[evandro]

lib/Target/AArch64/AArch64ISelLowering.cpp

5448

OK. Please, add a TODO comment here about it before pushing the patch.

Thank you.

lib/Target/AArch64/AArch64ISelLowering.cpp

 bool AArch64TargetLowering::isOffsetFoldingLegal(
     const GlobalAddressSDNode *GA) const {
   // Offsets are folded in the DAG combine rather than here so that we can
   // intelligently choose an offset based on the uses.
   return false;
 }
 
 bool AArch64TargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT) const {
-  // We can materialize #0.0 as fmov $Rd, XZR for 64-bit and 32-bit cases.
-  // FIXME: We should be able to handle f128 as well with a clever lowering.
-  if (Imm.isPosZero() && (VT == MVT::f64 || VT == MVT::f32 ||
-                          (VT == MVT::f16 && Subtarget->hasFullFP16()))) {
-    LLVM_DEBUG(dbgs() << "Legal " << VT.getEVTString() << " imm value: 0\n");
-    return true;
-  }
-
   bool IsLegal = false;
   SmallString<128> ImmStrVal;
   Imm.toString(ImmStrVal);
 
+  // FIXME: We should be able to handle f128 as well with a clever lowering.
+  const APInt ImmInt = Imm.bitcastToAPInt();
+  bool IsVTLegal = true;
   if (VT == MVT::f64)
-    IsLegal = AArch64_AM::getFP64Imm(Imm) != -1;
+    IsLegal = AArch64_AM::getFP64Imm(ImmInt) != -1;
   else if (VT == MVT::f32)
-    IsLegal = AArch64_AM::getFP32Imm(Imm) != -1;
+    IsLegal = AArch64_AM::getFP32Imm(ImmInt) != -1;
   else if (VT == MVT::f16 && Subtarget->hasFullFP16())
-    IsLegal = AArch64_AM::getFP16Imm(Imm) != -1;
+    IsLegal = AArch64_AM::getFP16Imm(ImmInt) != -1;
+  else
+    IsVTLegal = false;
 
-  if (IsLegal) {
-    LLVM_DEBUG(dbgs() << "Legal " << VT.getEVTString()
-                      << " imm value: " << ImmStrVal << "\n");
-    return true;
-  }
+  // If we can not materialize in immediate field for fmov, check if the
+  // value can be encoded as the immediate operanf of a logical instruction.
+  if (!IsLegal && IsVTLegal)
+    // We can materialize #0.0 as fmov $Rd, XZR for 64-bit and 32-bit cases.
+    IsLegal = Imm.isPosZero() ||
+              AArch64_AM::isLogicalImmediate(ImmInt.getZExtValue(),
+                                             VT.getSizeInBits());
 
-  LLVM_DEBUG(dbgs() << "Illegal " << VT.getEVTString()
+  LLVM_DEBUG(dbgs() << (IsLegal ? "Legal " : "Illegal ") << VT.getEVTString()
                     << " imm value: " << ImmStrVal << "\n");
-  return false;
+  return IsLegal;
 }
 
 //===----------------------------------------------------------------------===//
 //                          AArch64 Optimization Hooks
 //===----------------------------------------------------------------------===//
 
 static SDValue getEstimate(const AArch64Subtarget *ST, unsigned Opcode,
                            SDValue Operand, SelectionDAG &DAG,
                            int &ExtraSteps) {

test/CodeGen/AArch64/fabs.ll

; CHECK-NEXT:    ret
   ret double %cond
 }
 
 ; Try again with different type, predicate, and compare constant.
 
 define float @still_not_fabs(float %x) #0 {
 ; CHECK-LABEL: still_not_fabs:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    adrp x8, .LCPI1_0
-; CHECK-NEXT:    ldr s1, [x8, :lo12:.LCPI1_0]
-; CHECK-NEXT:    fneg s2, s0
-; CHECK-NEXT:    fcmp s0, s1
-; CHECK-NEXT:    fcsel s0, s0, s2, ge
+; CHECK-NEXT:    orr w8, wzr, #0x80000000
+; CHECK-NEXT:    fmov s2, w8
+; CHECK-NEXT:    fneg s1, s0
+; CHECK-NEXT:    fcmp s0, s2
+; CHECK-NEXT:    fcsel s0, s0, s1, ge
 ; CHECK-NEXT:    ret
   %cmp = fcmp nnan oge float %x, -0.0
   %sub = fsub nnan float -0.0, %x
   %cond = select i1 %cmp, float %x, float %sub
   ret float %cond
 }
 
 define float @nabsf(float %a) {

test/CodeGen/AArch64/isinf.ll

+; RUN: llc -mtriple=aarch64-none-linux-gnu -mattr=+neon < %s -o -| FileCheck %s
+
+declare float  @llvm.fabs.f32(float)
+declare double @llvm.fabs.f64(double)
+declare fp128  @llvm.fabs.f128(fp128)
+
+; Check if INFINITY for float is materialized
+define i32 @replace_isinf_call_f32(float %x) {
+; CHECK-LABEL: replace_isinf_call_f32:
+; CHECK:       orr    [[INFSCALARREG:w[0-9]+]], wzr, #0x7f800000
+; CHECK-NEXT:  fabs   [[ABS:s[0-9]+]], s0
+; CHECK-NEXT:  fmov   [[INFREG:s[0-9]+]], [[INFSCALARREG]]
+; CHECK-NEXT:  fcmp   [[ABS]], [[INFREG]]
+; CHECK-NEXT:  cset   w0, eq
+  %abs = tail call float @llvm.fabs.f32(float %x)
+  %cmpinf = fcmp oeq float %abs, 0x7FF0000000000000
+  %ret = zext i1 %cmpinf to i32
+  ret i32 %ret
+}
+
+; Check if INFINITY for double is materialized
+define i32 @replace_isinf_call_f64(double %x) {
+; CHECK-LABEL: replace_isinf_call_f64:
+; CHECK:       orr    [[INFSCALARREG:x[0-9]+]], xzr, #0x7ff0000000000000
+; CHECK-NEXT:  fabs   [[ABS:d[0-9]+]], d0
+; CHECK-NEXT:  fmov   [[INFREG:d[0-9]+]], [[INFSCALARREG]]
+; CHECK-NEXT:  fcmp   [[ABS]], [[INFREG]]
+; CHECK-NEXT:  cset   w0, eq
+  %abs = tail call double @llvm.fabs.f64(double %x)
+  %cmpinf = fcmp oeq double %abs, 0x7FF0000000000000
+  %ret = zext i1 %cmpinf to i32
+  ret i32 %ret
+}
+
+; For long double it still requires loading the constant.
+define i32 @replace_isinf_call_f128(fp128 %x) {
+; CHECK-LABEL: replace_isinf_call_f128:
+; CHECK:       adrp    [[ADDR:x[0-9]+]], [[CSTLABEL:.LCP.*]]
+; CHECK:       ldr     q1, {{[[]}}[[ADDR]], :lo12:[[CSTLABEL]]{{[]]}}
+; CHECK:       bl      __eqtf2
+; CHECK:       cmp     w0, #0
+; CHECK:       cset    w0, eq
+  %abs = tail call fp128 @llvm.fabs.f128(fp128 %x)
+  %cmpinf = fcmp oeq fp128 %abs, 0xL00000000000000007FFF000000000000
+  %ret = zext i1 %cmpinf to i32
+  ret i32 %ret
+}

test/CodeGen/AArch64/known-never-nan.ll

; CHECK-NEXT:    ret
   ret float %val
 }
 
 ; If f1 is 0, fmul is NaN because 0.0 * -INF = NaN
 ; Therefore, this is not fmaxnm.
 define float @not_fmaxnm_maybe_nan(i32 %i1, i32 %i2) #0 {
 ; CHECK-LABEL: not_fmaxnm_maybe_nan:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    adrp x8, .LCPI1_0
-; CHECK-NEXT:    ldr s0, [x8, :lo12:.LCPI1_0]
-; CHECK-NEXT:    ucvtf s1, w0
-; CHECK-NEXT:    ucvtf s2, w1
-; CHECK-NEXT:    fmov s3, #17.00000000
-; CHECK-NEXT:    fmul s0, s1, s0
-; CHECK-NEXT:    fadd s1, s2, s3
+; CHECK-NEXT:    orr w8, wzr, #0xff800000
+; CHECK-NEXT:    ucvtf s0, w0
+; CHECK-NEXT:    ucvtf s1, w1
+; CHECK-NEXT:    fmov s2, #17.00000000
+; CHECK-NEXT:    fmov s3, w8
+; CHECK-NEXT:    fmul s0, s0, s3
+; CHECK-NEXT:    fadd s1, s1, s2
 ; CHECK-NEXT:    fcmp s0, s1
 ; CHECK-NEXT:    fcsel s0, s0, s1, pl
 ; CHECK-NEXT:    ret
   %f1 = uitofp i32 %i1 to float
   %fmul = fmul float %f1, 0xfff0000000000000 ; -INFINITY as 64-bit hex
   %f2 = uitofp i32 %i2 to float
   %fadd2 = fadd float %f2, 17.0
   %cmp = fcmp uge float %fmul, %fadd2
   %val = select i1 %cmp, float %fmul, float %fadd2
   ret float %val
 }
 
 attributes #0 = { "no-signed-zeros-fp-math"="true" }

test/CodeGen/AArch64/win_cst_pool.ll

 ; RUN: llc < %s -mtriple=aarch64-win32-msvc | FileCheck %s
 ; RUN: llc < %s -mtriple=aarch64-win32-gnu | FileCheck -check-prefix=MINGW %s
 
 define double @double() {
-  ret double 0x0000000000800000
+  ret double 0x0000000000800001
 }
-; CHECK:              .globl  __real@0000000000800000
-; CHECK-NEXT:         .section        .rdata,"dr",discard,__real@0000000000800000
+; CHECK:              .globl  __real@0000000000800001
+; CHECK-NEXT:         .section        .rdata,"dr",discard,__real@0000000000800001
 ; CHECK-NEXT:         .p2align  3
-; CHECK-NEXT: __real@0000000000800000:
-; CHECK-NEXT:         .xword   8388608
+; CHECK-NEXT: __real@0000000000800001:
+; CHECK-NEXT:         .xword   8388609
 ; CHECK:      double:
-; CHECK:               adrp    x8, __real@0000000000800000
-; CHECK-NEXT:          ldr     d0, [x8, __real@0000000000800000]
+; CHECK:               adrp    x8, __real@0000000000800001
+; CHECK-NEXT:          ldr     d0, [x8, __real@0000000000800001]
 ; CHECK-NEXT:          ret
 
 ; MINGW:              .section        .rdata,"dr"
 ; MINGW-NEXT:         .p2align  3
 ; MINGW-NEXT: [[LABEL:\.LC.*]]:
-; MINGW-NEXT:         .xword   8388608
+; MINGW-NEXT:         .xword   8388609
 ; MINGW:      double:
 ; MINGW:               adrp    x8, [[LABEL]]
 ; MINGW-NEXT:          ldr     d0, [x8, [[LABEL]]]
 ; MINGW-NEXT:          ret