[DAGCombine] Do not remove masking argument to FP16_TO_FP for some targets

[nemanjai]

Summary

As of commit 284f2bffc9bc5, the DAG Combiner gets rid of the masking of the input to this node if the mask only keeps the bottom 16 bits. This is because the underlying library function does not use the high order bits. However, on PowerPC's ELFv2 ABI, it is the caller that is responsible for clearing the bits from the register. Therefore, the library implementation of __gnu_h2f_ieee will return an incorrect result if the bits aren't cleared.

This combine is desired for ARM (and possibly other targets) so this patch adds a query to Target Lowering to check if this zeroing needs to be kept.

I am hoping to be able to get this reviewed, updated if needed and merge the fix into 12.0.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=49092

[t.p.northover]

We already seem to have shouldSignExtendTypeInLibCall (used in the actual ExpandLibCall this code gets to) and shouldExtendTypeInLibCall (used in another expander I didn't know about). Ideally that's where we'd fix this issue by getting the generic call lowerer to insert a zext (which may be folded again later, if it's a sound transformation). Unfortunately because we only have an i32 when the expansion happens I don't think that's possible.

But this seems like a much more targetted fix that only applies to this one f16 conversion so the name should probably reflect that.

[nemanjai]

Correct the formatting and change the name to reflect this is for the half precision conversion libcall.

[t.p.northover]

Thanks. I think this looks like the best fix we can do now.

[nemanjai]

In D96283#2550702, @t.p.northover wrote:

We already seem to have shouldSignExtendTypeInLibCall (used in the actual ExpandLibCall this code gets to) and shouldExtendTypeInLibCall (used in another expander I didn't know about). Ideally that's where we'd fix this issue by getting the generic call lowerer to insert a zext (which may be folded again later, if it's a sound transformation). Unfortunately because we only have an i32 when the expansion happens I don't think that's possible.

My initial thought was along these lines. In fact, I tried to do this in the PPC call lowering but no information exists at that point that this should be a 16-bit value. This would kind of require that I use the name of the function or something similarly flaky which I don't find acceptable. Ultimately, the combine is quite specific (the masking of arguments to normal calls is not removed) so the fix had to be quite specific.

But this seems like a much more targetted fix that only applies to this one f16 conversion so the name should probably reflect that.

Updated the name. My initial thought was to make it a somewhat generic name in case this is useful for other similar transformations in the future.

[nemanjai]

In D96283#2550813, @t.p.northover wrote:

Thanks. I think this looks like the best fix we can do now.

Thank you so much for the quick review Tim.

llvm/include/llvm/CodeGen/TargetLowering.h

public:
   virtual bool shouldConsiderGEPOffsetSplit() const { return false; }
 
   /// Return true if creating a shift of the type by the given
   /// amount is not profitable.
   virtual bool shouldAvoidTransformToShift(EVT VT, unsigned Amount) const {
     return false;
   }
 
+  /// Does this target tolerate non-zero high-order bits in a register passed
+  /// to a library function.
+  virtual bool shouldKeepZExtForLibCall() const {
+    return false;
+  }
+
   //===--------------------------------------------------------------------===//
   // Runtime Library hooks
   //
 
   /// Rename the default libcall routine name for the specified libcall.
   void setLibcallName(RTLIB::Libcall Call, const char *Name) {
     LibcallRoutineNames[Call] = Name;
   }

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitFP_TO_FP16(SDNode *N) {
 
   return SDValue();
 }
 
 SDValue DAGCombiner::visitFP16_TO_FP(SDNode *N) {
   SDValue N0 = N->getOperand(0);
 
   // fold fp16_to_fp(op & 0xffff) -> fp16_to_fp(op)
-  if (N0->getOpcode() == ISD::AND) {
+  if (!TLI.shouldKeepZExtForLibCall() && N0->getOpcode() == ISD::AND) {
     ConstantSDNode *AndConst = getAsNonOpaqueConstant(N0.getOperand(1));
     if (AndConst && AndConst->getAPIntValue() == 0xffff) {
       return DAG.getNode(ISD::FP16_TO_FP, SDLoc(N), N->getValueType(0),
                          N0.getOperand(0));
     }
   }
 
   return SDValue();

llvm/lib/Target/PowerPC/PPCISelLowering.h

public:
 
     const MCPhysReg *getScratchRegisters(CallingConv::ID CC) const override;
 
     // Should we expand the build vector with shuffles?
     bool
     shouldExpandBuildVectorWithShuffles(EVT VT,
                                         unsigned DefinedValues) const override;
 
+    // Keep the zero-extensions for arguments to libcalls.
+    bool shouldKeepZExtForLibCall() const override {
+      return true;
+    }
+
     /// createFastISel - This method returns a target-specific FastISel object,
     /// or null if the target does not support "fast" instruction selection.
     FastISel *createFastISel(FunctionLoweringInfo &FuncInfo,
                              const TargetLibraryInfo *LibInfo) const override;
 
     /// Returns true if an argument of type Ty needs to be passed in a
     /// contiguous block of registers in calling convention CallConv.
     bool functionArgumentNeedsConsecutiveRegisters(

llvm/test/CodeGen/PowerPC/pr49092.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mcpu=pwr8 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr \
+; RUN:   -mtriple=powerpc64le-unknown-unknown < %s | FileCheck %s
+; RUN: llc -mcpu=pwr9 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr \
+; RUN:   -mtriple=powerpc64le-unknown-unknown < %s | FileCheck %s \
+; RUN:   -check-prefix=CHECK-P9
+
+define dso_local half @test2(i64 %a, i64 %b) local_unnamed_addr #0 {
+; CHECK-LABEL: test2:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    mflr r0
+; CHECK-NEXT:    std r0, 16(r1)
+; CHECK-NEXT:    stdu r1, -32(r1)
+; CHECK-NEXT:    add r3, r4, r3
+; CHECK-NEXT:    addi r3, r3, 11
+; CHECK-NEXT:    clrlwi r3, r3, 16
+; CHECK-NEXT:    bl __gnu_h2f_ieee
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    addi r1, r1, 32
+; CHECK-NEXT:    ld r0, 16(r1)
+; CHECK-NEXT:    mtlr r0
+; CHECK-NEXT:    blr
+;
+; CHECK-P9-LABEL: test2:
+; CHECK-P9:       # %bb.0: # %entry
+; CHECK-P9-NEXT:    add r3, r4, r3
+; CHECK-P9-NEXT:    addi r3, r3, 11
+; CHECK-P9-NEXT:    clrlwi r3, r3, 16
+; CHECK-P9-NEXT:    mtfprwz f0, r3
+; CHECK-P9-NEXT:    xscvhpdp f1, f0
+; CHECK-P9-NEXT:    blr
+entry:
+  %add = add i64 %b, %a
+  %0 = trunc i64 %add to i16
+  %conv = add i16 %0, 11
+  %call = bitcast i16 %conv to half
+  ret half %call
+}
+attributes #0 = { nounwind }