[GlobalISel] Change widenScalar of G_FCONSTANT to mutate into G_CONSTANT.

[aemerson]

Summary

Widening a G_FCONSTANT by extending and then generating G_FPTRUNC doesn't produce the same result all the time. Instead, we can just transform it to a G_CONSTANT of the same bit pattern and truncate using a plain G_TRUNC instead.

Fixes https://github.com/llvm/llvm-project/issues/56454

[foad]

Widening a G_FCONSTANT by extending and then generating G_FPTRUNC doesn't produce the same result all the time.

That's only a problem if you care about the bit pattern of NaNs, right? It should be OK for all non-NaN values.

[aemerson]

In D129743#3651111, @foad wrote:

Widening a G_FCONSTANT by extending and then generating G_FPTRUNC doesn't produce the same result all the time.

That's only a problem if you care about the bit pattern of NaNs, right? It should be OK for all non-NaN values.

Yeah, but it seems better to handle this as a G_CONSTANT rather than have value dependent legalization.

[foad]

In D129743#3651128, @aemerson wrote:

In D129743#3651111, @foad wrote:

Widening a G_FCONSTANT by extending and then generating G_FPTRUNC doesn't produce the same result all the time.

That's only a problem if you care about the bit pattern of NaNs, right? It should be OK for all non-NaN values.

Yeah, but it seems better to handle this as a G_CONSTANT rather than have value dependent legalization.

Agreed, I wasn't suggesting to change anything, just trying to understand what the problem was.

I don't really understand why we have G_FCONSTANT in the first place. Also I don't really understand the concept of legalizing a constant.

[aemerson]

In D129743#3651143, @foad wrote:

In D129743#3651128, @aemerson wrote:

In D129743#3651111, @foad wrote:

Widening a G_FCONSTANT by extending and then generating G_FPTRUNC doesn't produce the same result all the time.

That's only a problem if you care about the bit pattern of NaNs, right? It should be OK for all non-NaN values.

Yeah, but it seems better to handle this as a G_CONSTANT rather than have value dependent legalization.

Agreed, I wasn't suggesting to change anything, just trying to understand what the problem was.

I don't really understand why we have G_FCONSTANT in the first place. Also I don't really understand the concept of legalizing a constant.

It probably makes selection easier when importing patterns, so that's something that might cause some issues if we decide to remove it one day.

[arsenm]

Could use some additional end to end tests, particularly ones that change the value (and some cases where it folds through an fpext to a legalized operation). We might be missing some constant fold code that bothers handling G_CONSTANT and G_FCONSTANT interchangeably.

[aemerson]

I added a couple of tests but not exactly sure what you meant by "folds through fpext". I guess at the moment we don't have any constant folding that will transform G_FPEXT(G_CONSTANT) -> wide G_CONSTANT.

[arsenm]

In D129743#3652626, @aemerson wrote:

I added a couple of tests but not exactly sure what you meant by "folds through fpext". I guess at the moment we don't have any constant folding that will transform G_FPEXT(G_CONSTANT) -> wide G_CONSTANT.

Yes that's what I mean (although I guess that could turn back into G_FCONSTANT)

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

case TargetOpcode::G_CONSTANT: {
     Observer.changingInstr(MI);
     SrcMO.setCImm(ConstantInt::get(Ctx, Val));
 
     widenScalarDst(MI, WideTy);
     Observer.changedInstr(MI);
     return Legalized;
   }
   case TargetOpcode::G_FCONSTANT: {
+    // To avoid changing the bits of the constant due to extension to a larger
+    // type and then using G_FPTRUNC, we simply convert to a G_CONSTANT.
     MachineOperand &SrcMO = MI.getOperand(1);
-    LLVMContext &Ctx = MIRBuilder.getMF().getFunction().getContext();
-    APFloat Val = SrcMO.getFPImm()->getValueAPF();
-    bool LosesInfo;
-    switch (WideTy.getSizeInBits()) {
-    case 32:
-      Val.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven,
-                  &LosesInfo);
-      break;
-    case 64:
-      Val.convert(APFloat::IEEEdouble(), APFloat::rmNearestTiesToEven,
-                  &LosesInfo);
-      break;
-    default:
-      return UnableToLegalize;
-    }
-
-    assert(!LosesInfo && "extend should always be lossless");
-
-    Observer.changingInstr(MI);
-    SrcMO.setFPImm(ConstantFP::get(Ctx, Val));
-
-    widenScalarDst(MI, WideTy, 0, TargetOpcode::G_FPTRUNC);
-    Observer.changedInstr(MI);
+    APInt Val = SrcMO.getFPImm()->getValueAPF().bitcastToAPInt();
+    MIRBuilder.setInstrAndDebugLoc(MI);
+    auto IntCst = MIRBuilder.buildConstant(MI.getOperand(0).getReg(), Val);
+    widenScalarDst(*IntCst, WideTy, 0, TargetOpcode::G_TRUNC);
+    MI.eraseFromParent();
     return Legalized;
   }
   case TargetOpcode::G_IMPLICIT_DEF: {
     Observer.changingInstr(MI);
     widenScalarDst(MI, WideTy);
     Observer.changedInstr(MI);
     return Legalized;
   }

llvm/test/CodeGen/AArch64/GlobalISel/legalize-constant.mir

bb.0:
     liveins: $x0
     ; CHECK-LABEL: name: test_fconstant
     ; CHECK: liveins: $x0
     ; CHECK-NEXT: {{  $}}
     ; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_FCONSTANT float 1.000000e+00
     ; CHECK-NEXT: $w0 = COPY [[C]](s32)
     ; CHECK-NEXT: [[C1:%[0-9]+]]:_(s64) = G_FCONSTANT double 2.000000e+00
     ; CHECK-NEXT: $x0 = COPY [[C1]](s64)
-    ; CHECK-NEXT: [[C2:%[0-9]+]]:_(s32) = G_FCONSTANT float 0.000000e+00
-    ; CHECK-NEXT: [[FPTRUNC:%[0-9]+]]:_(s16) = G_FPTRUNC [[C2]](s32)
-    ; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(s32) = G_ANYEXT [[FPTRUNC]](s16)
-    ; CHECK-NEXT: $w0 = COPY [[ANYEXT]](s32)
+    ; CHECK-NEXT: [[C2:%[0-9]+]]:_(s32) = G_CONSTANT i32 0
+    ; CHECK-NEXT: $w0 = COPY [[C2]](s32)
     %0:_(s32) = G_FCONSTANT float 1.0
     $w0 = COPY %0
     %1:_(s64) = G_FCONSTANT double 2.0
     $x0 = COPY %1
     %2:_(s16) = G_FCONSTANT half 0.0
     %3:_(s32) = G_ANYEXT %2
     $w0 = COPY %3
 ...

llvm/test/CodeGen/AArch64/GlobalISel/legalize-fp16-fconstant.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple aarch64-unknown-unknown -verify-machineinstrs -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=NO-FP16
 # RUN: llc -mtriple aarch64-unknown-unknown -verify-machineinstrs -mattr=+fullfp16 -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=FP16
 
 ---
 name:            fp16
 tracksRegLiveness: true
 body: |
   bb.0:
     ; NO-FP16-LABEL: name: fp16
-    ; NO-FP16: [[C:%[0-9]+]]:_(s32) = G_FCONSTANT float 0.000000e+00
-    ; NO-FP16-NEXT: %cst:_(s16) = G_FPTRUNC [[C]](s32)
+    ; NO-FP16: %cst:_(s16) = G_CONSTANT i16 0
     ; NO-FP16-NEXT: $h0 = COPY %cst(s16)
     ; NO-FP16-NEXT: RET_ReallyLR implicit $h0
     ; FP16-LABEL: name: fp16
     ; FP16: %cst:_(s16) = G_FCONSTANT half 0xH0000
     ; FP16-NEXT: $h0 = COPY %cst(s16)
     ; FP16-NEXT: RET_ReallyLR implicit $h0
     %cst:_(s16) = G_FCONSTANT half 0.0
     $h0 = COPY %cst
     RET_ReallyLR implicit $h0
+...
+---
+name:            fp16_non_zero
+tracksRegLiveness: true
+body: |
+  bb.0:
+    ; NO-FP16-LABEL: name: fp16_non_zero
+    ; NO-FP16: %cst:_(s16) = G_CONSTANT i16 16384
+    ; NO-FP16-NEXT: $h0 = COPY %cst(s16)
+    ; NO-FP16-NEXT: RET_ReallyLR implicit $h0
+    ; FP16-LABEL: name: fp16_non_zero
+    ; FP16: %cst:_(s16) = G_FCONSTANT half 0xH4000
+    ; FP16-NEXT: $h0 = COPY %cst(s16)
+    ; FP16-NEXT: RET_ReallyLR implicit $h0
+    %cst:_(s16) = G_FCONSTANT half 2.0
+    $h0 = COPY %cst
+    RET_ReallyLR implicit $h0
+...
+---
+name:            nan
+tracksRegLiveness: true
+body:             |
+  bb.1.entry:
+    ; NO-FP16-LABEL: name: nan
+    ; NO-FP16: %cst:_(s16) = G_CONSTANT i16 31745
+    ; NO-FP16-NEXT: %ext:_(s32) = G_FPEXT %cst(s16)
+    ; NO-FP16-NEXT: $w0 = COPY %ext(s32)
+    ; NO-FP16-NEXT: RET_ReallyLR implicit $w0
+    ; FP16-LABEL: name: nan
+    ; FP16: %cst:_(s16) = G_FCONSTANT half 0xH7C01
+    ; FP16-NEXT: %ext:_(s32) = G_FPEXT %cst(s16)
+    ; FP16-NEXT: $w0 = COPY %ext(s32)
+    ; FP16-NEXT: RET_ReallyLR implicit $w0
+    %cst:_(s16) = G_FCONSTANT half 0xH7C01
+    %ext:_(s32) = G_FPEXT %cst(s16)
+    $w0 = COPY %ext(s32)
+    RET_ReallyLR implicit $w0
 
+...