[AArch64][GlobalISel] Change G_FCONSTANTs feeding into stores into G_CONSTANTS

[paquette]

Summary

Given the following situation:

x = G_FCONSTANT (something that can't be materialized)
G_STORE x, some_addr

We know that x must be materialized as at least a single mov. However, at the time of selection, the G_STORE will have been regbankselected to a FPR store.

So, as a result, you'll get an unnecessary fmov into the G_STORE.

Storing a constant value in a GPR and a constant value in a FPR are the same. So, whenever you see a G_FCONSTANT that feeds into only G_STORES, so might as well make it a G_CONSTANT.

This adds a target-specific combine which changes G_FCONSTANTs feeding into G_STOREs into G_CONSTANTs.

[paquette]

Also I have no idea if this is how we want to do target-specific combines, so if there's a better way to do it, I'll take it.

[arsenm]

llvm/lib/Target/AArch64/AArch64PreLegalizerCombiner.cpp

50

There's no reason to go through getZExtValue, buildConstant accepts the APInt directly

[aemerson]

I think in cases where it's possible to use an fmov with immediate:

fmov s0, #fpimm
str s0, [x0]

We should do that over forcing going through GPR. When we go through gpr, there might be fp immediate values which cannot be represented with a single instruction GPR register materialization, instead requiring movk/movz.

[paquette]

When we go through gpr, there might be fp immediate values which cannot be represented with a single instruction GPR register materialization, instead requiring movk/movz.

I don't think that we can ever actually hit this situation.

In AArch64AddressingModes.h:

/// getFP32Imm - Return an 8-bit floating-point version of the 32-bit
/// floating-point value. If the value cannot be represented as an 8-bit
/// floating-point value, then return -1.
...
/// getFP64Imm - Return an 8-bit floating-point version of the 64-bit
/// floating-point value. If the value cannot be represented as an 8-bit
/// floating-point value, then return -1.

The range of legal floating point values for fmov is much smaller than the range of legal values we can pack into a mov. So, I don't think a case where we can avoid two movs by using fmov actually exists.

[paquette]

• Don't use getZExtValue
• Add asserts that we're getting a G_FCONSTANT

[aemerson]

LGTM.

llvm/lib/Target/AArch64/AArch64Combine.td

 //=- AArch64.td - Define AArch64 Combine Rules ---------------*- tablegen -*-=//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //
 //===----------------------------------------------------------------------===//
 
 include "llvm/Target/GlobalISel/Combine.td"
 
+def fconstant_to_constant : GICombineRule<
+  (defs root:$root),
+  (match (wip_match_opcode G_FCONSTANT):$root,
+         [{ return matchFConstantToConstant(*${root}, MRI); }]),
+  (apply [{ applyFConstantToConstant(*${root}); }])>;
+
 def AArch64PreLegalizerCombinerHelper: GICombinerHelper<
   "AArch64GenPreLegalizerCombinerHelper", [all_combines,
-                                           elide_br_by_inverting_cond]> {
+                                           elide_br_by_inverting_cond,
+                                           fconstant_to_constant]> {
   let DisableRuleOption = "aarch64prelegalizercombiner-disable-rule";
 }

llvm/lib/Target/AArch64/AArch64PreLegalizerCombiner.cpp

 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "aarch64-prelegalizer-combiner"
 
 using namespace llvm;
 using namespace MIPatternMatch;
 
+/// Return true if a G_FCONSTANT instruction is known to be better-represented
+/// as a G_CONSTANT.
+static bool matchFConstantToConstant(MachineInstr &MI,
+                                     MachineRegisterInfo &MRI) {
+  assert(MI.getOpcode() == TargetOpcode::G_FCONSTANT);
+  Register DstReg = MI.getOperand(0).getReg();
+  const unsigned DstSize = MRI.getType(DstReg).getSizeInBits();
+  if (DstSize != 32 && DstSize != 64)
+    return false;
+
+  // When we're storing a value, it doesn't matter what register bank it's on.
+  // Since not all floating point constants can be materialized using a fmov,
+  // it makes more sense to just use a GPR.
+  return all_of(MRI.use_instructions(DstReg),
+                [](const MachineInstr &Use) { return Use.mayStore(); });
+}
+
+/// Change a G_FCONSTANT into a G_CONSTANT.
+static void applyFConstantToConstant(MachineInstr &MI) {
+  assert(MI.getOpcode() == TargetOpcode::G_FCONSTANT);
+  MachineIRBuilder MIB(MI);
+  const APFloat &ImmValAPF = MI.getOperand(1).getFPImm()->getValueAPF();
+  MIB.buildConstant(MI.getOperand(0).getReg(), ImmValAPF.bitcastToAPInt());
+  MI.eraseFromParent();
+}
+
 #define AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_DEPS
 #include "AArch64GenGICombiner.inc"
 #undef AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_DEPS
 
 namespace {
 #define AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_H
 #include "AArch64GenGICombiner.inc"
 #undef AARCH64PRELEGALIZERCOMBINERHELPER_GENCOMBINERHELPER_H

llvm/test/CodeGen/AArch64/GlobalISel/combine-fconstant.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -run-pass=aarch64-prelegalizer-combiner -verify-machineinstrs -mtriple aarch64-unknown-unknown %s -o - | FileCheck %s
+...
+---
+name:            fconstant_to_constant_s32
+alignment:       4
+tracksRegLiveness: true
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $x0
+    ; Only feeding into stores here. Also, the value can't be materialized using
+    ; fmov, so it's strictly better to use a mov.
+    ; CHECK-LABEL: name: fconstant_to_constant_s32
+    ; CHECK: liveins: $x0
+    ; CHECK: [[COPY:%[0-9]+]]:_(p0) = COPY $x0
+    ; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 1028443341
+    ; CHECK: [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 524
+    ; CHECK: [[PTR_ADD:%[0-9]+]]:_(p0) = G_PTR_ADD [[COPY]], [[C1]](s64)
+    ; CHECK: G_STORE [[C]](s32), [[PTR_ADD]](p0) :: (store 4)
+    ; CHECK: RET_ReallyLR
+    %0:_(p0) = COPY $x0
+    %3:_(s32) = G_FCONSTANT float 0x3FA99999A0000000
+    %1:_(s64) = G_CONSTANT i64 524
+    %2:_(p0) = G_PTR_ADD %0, %1(s64)
+    G_STORE %3(s32), %2(p0) :: (store 4)
+    RET_ReallyLR
+...
+---
+name:            fconstant_to_constant_s64
+alignment:       4
+tracksRegLiveness: true
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $x0
+    ; CHECK-LABEL: name: fconstant_to_constant_s64
+    ; CHECK: liveins: $x0
+    ; CHECK: %ptr:_(p0) = COPY $x0
+    ; CHECK: %c:_(s64) = G_CONSTANT i64 0
+    ; CHECK: G_STORE %c(s64), %ptr(p0) :: (store 8)
+    ; CHECK: RET_ReallyLR
+    %ptr:_(p0) = COPY $x0
+    %c:_(s64) = G_FCONSTANT double 0.0
+    G_STORE %c(s64), %ptr(p0) :: (store 8)
+    RET_ReallyLR
+...
+---
+name:            no_store_means_no_combine
+alignment:       4
+tracksRegLiveness: true
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $x0, $x1
+    ; When we aren't feeding into a store, the combine shouldn't happen.
+    ; CHECK-LABEL: name: no_store_means_no_combine
+    ; CHECK: liveins: $x0, $x1
+    ; CHECK: %v:_(s64) = COPY $x0
+    ; CHECK: %c:_(s64) = G_FCONSTANT double 0.000000e+00
+    ; CHECK: %add:_(s64) = G_FADD %v, %c
+    ; CHECK: RET_ReallyLR implicit %add(s64)
+    %v:_(s64) = COPY $x0
+    %c:_(s64) = G_FCONSTANT double 0.0
+    %add:_(s64) = G_FADD %v, %c
+    RET_ReallyLR implicit %add
+...