[llvm] r178438 - Cleanup PPC(64) i32 -> float/double conversion

Bill Schmidt wschmidt at linux.vnet.ibm.com
Mon Apr 1 08:30:55 PDT 2013


On Mon, 2013-04-01 at 10:22 -0500, Hal Finkel wrote:
> ----- Original Message -----
> > From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: llvm-commits at cs.uiuc.edu
> > Sent: Monday, April 1, 2013 10:08:54 AM
> > Subject: Re: [llvm] r178438 - Cleanup PPC(64) i32 -> float/double conversion
> > 
> > On Sun, 2013-03-31 at 09:50 -0500, Hal Finkel wrote:
> > > 
> > > ----- Original Message -----
> > > > From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> > > > To: "Hal Finkel" <hfinkel at anl.gov>
> > > > Cc: llvm-commits at cs.uiuc.edu
> > > > Sent: Sunday, March 31, 2013 9:20:36 AM
> > > > Subject: Re: [llvm] r178438 - Cleanup PPC(64) i32 -> float/double
> > > > conversion
> > > > 
> > > > Hi Hal,
> > > > 
> > > > On Sun, 2013-03-31 at 01:58 +0000, Hal Finkel wrote:
> > > > > Author: hfinkel
> > > > > Date: Sat Mar 30 20:58:02 2013
> > > > > New Revision: 178438
> > > > > 
> > > > > URL: http://llvm.org/viewvc/llvm-project?rev=178438&view=rev
> > > > > Log:
> > > > > Cleanup PPC(64) i32 -> float/double conversion
> > > > > 
> > > > > The existing SINT_TO_FP code for i32 -> float/double conversion
> > > > > was
> > > > > disabled
> > > > > because it relied on broken EXTSW_32/STD_32 instruction
> > > > > definitions. The
> > > > > original intent had been to enable these 64-bit instructions to
> > > > > be
> > > > > used on CPUs
> > > > > that support them even in 32-bit mode.  Unfortunately, this
> > > > > form of
> > > > > lying to
> > > > > the infrastructure was buggy (as explained in the FIXME
> > > > > comment)
> > > > > and had
> > > > > therefore been disabled.
> > > > > 
> > > > > This re-enables this functionality, using regular DAG nodes,
> > > > > but
> > > > > only when
> > > > > compiling in 64-bit mode. The old STD_32/EXTSW_32 definitions
> > > > > (which were dead)
> > > > > are removed.
> > > > > 
> > > > > Added:
> > > > >     llvm/trunk/test/CodeGen/PowerPC/i32-to-float.ll
> > > > > Modified:
> > > > >     llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
> > > > >     llvm/trunk/lib/Target/PowerPC/PPCISelLowering.h
> > > > >     llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td
> > > > >     llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.td
> > > > >     llvm/trunk/lib/Target/PowerPC/PPCRegisterInfo.cpp
> > > > > 
> > > > > Modified: llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp?rev=178438&r1=178437&r2=178438&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
> > > > > (original)
> > > > > +++ llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp Sat Mar
> > > > > 30
> > > > > 20:58:02 2013
> > > > > @@ -326,10 +326,8 @@ PPCTargetLowering::PPCTargetLowering(PPC
> > > > >      // We cannot do this with Promote because i64 is not a
> > > > >      legal
> > > > >      type.
> > > > >      setOperationAction(ISD::FP_TO_UINT, MVT::i32, Custom);
> > > > > 
> > > > > -    // FIXME: disable this lowered code.  This generates
> > > > > 64-bit
> > > > > register values,
> > > > > -    // and we don't model the fact that the top part is
> > > > > clobbered
> > > > > by calls.  We
> > > > > -    // need to flag these together so that the value isn't
> > > > > live
> > > > > across a call.
> > > > > -    //setOperationAction(ISD::SINT_TO_FP, MVT::i32, Custom);
> > > > > +    if (Subtarget->isPPC64())
> > > > > +      setOperationAction(ISD::SINT_TO_FP, MVT::i32, Custom);
> > > > >    } else {
> > > > >      // PowerPC does not have FP_TO_UINT on 32-bit
> > > > >      implementations.
> > > > >      setOperationAction(ISD::FP_TO_UINT, MVT::i32, Expand);
> > > > > @@ -592,8 +590,6 @@ const char *PPCTargetLowering::getTarget
> > > > >    case PPCISD::SRL:             return "PPCISD::SRL";
> > > > >    case PPCISD::SRA:             return "PPCISD::SRA";
> > > > >    case PPCISD::SHL:             return "PPCISD::SHL";
> > > > > -  case PPCISD::EXTSW_32:        return "PPCISD::EXTSW_32";
> > > > > -  case PPCISD::STD_32:          return "PPCISD::STD_32";
> > > > >    case PPCISD::CALL:            return "PPCISD::CALL";
> > > > >    case PPCISD::CALL_NOP:        return "PPCISD::CALL_NOP";
> > > > >    case PPCISD::MTCTR:           return "PPCISD::MTCTR";
> > > > > @@ -4817,17 +4813,13 @@ SDValue
> > > > > PPCTargetLowering::LowerSINT_TO_
> > > > >    EVT PtrVT = DAG.getTargetLoweringInfo().getPointerTy();
> > > > >    SDValue FIdx = DAG.getFrameIndex(FrameIdx, PtrVT);
> > > > > 
> > > > > -  SDValue Ext64 = DAG.getNode(PPCISD::EXTSW_32, dl, MVT::i32,
> > > > > -                                Op.getOperand(0));
> > > > > +  SDValue Ext64 = DAG.getNode(ISD::SIGN_EXTEND, dl, MVT::i64,
> > > > > +                              Op.getOperand(0));
> > > > > 
> > > > >    // STD the extended value into the stack slot.
> > > > > -  MachineMemOperand *MMO =
> > > > > -
> > > > >    MF.getMachineMemOperand(MachinePointerInfo::getFixedStack(FrameIdx),
> > > > > -                            MachineMemOperand::MOStore, 8, 8);
> > > > > -  SDValue Ops[] = { DAG.getEntryNode(), Ext64, FIdx };
> > > > > -  SDValue Store =
> > > > > -    DAG.getMemIntrinsicNode(PPCISD::STD_32, dl,
> > > > > DAG.getVTList(MVT::Other),
> > > > > -                            Ops, 4, MVT::i64, MMO);
> > > > > +  SDValue Store = DAG.getStore(DAG.getEntryNode(), dl, Ext64,
> > > > > FIdx,
> > > > > +                               MachinePointerInfo(), false,
> > > > > false,
> > > > > 0);
> > > > > +
> > > > >    // Load the value as a double.
> > > > >    SDValue Ld = DAG.getLoad(MVT::f64, dl, Store, FIdx,
> > > > >    MachinePointerInfo(),
> > > > >                             false, false, false, 0);
> > > > > 
> > > > > Modified: llvm/trunk/lib/Target/PowerPC/PPCISelLowering.h
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCISelLowering.h?rev=178438&r1=178437&r2=178438&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/lib/Target/PowerPC/PPCISelLowering.h (original)
> > > > > +++ llvm/trunk/lib/Target/PowerPC/PPCISelLowering.h Sat Mar 30
> > > > > 20:58:02 2013
> > > > > @@ -91,10 +91,6 @@ namespace llvm {
> > > > >        /// code.
> > > > >        SRL, SRA, SHL,
> > > > > 
> > > > > -      /// EXTSW_32 - This is the EXTSW instruction for use
> > > > > with
> > > > > "32-bit"
> > > > > -      /// registers.
> > > > > -      EXTSW_32,
> > > > > -
> > > > >        /// CALL - A direct function call.
> > > > >        /// CALL_NOP is a call with the special NOP which
> > > > >        follows
> > > > >        64-bit
> > > > >        /// SVR4 calls.
> > > > > @@ -234,14 +230,11 @@ namespace llvm {
> > > > >        /// optimizations due to constant folding.
> > > > >        VADD_SPLAT,
> > > > > 
> > > > > -      /// STD_32 - This is the STD instruction for use with
> > > > > "32-bit" registers.
> > > > > -      STD_32 = ISD::FIRST_TARGET_MEMORY_OPCODE,
> > > > > -
> > > > >        /// CHAIN = STBRX CHAIN, GPRC, Ptr, Type - This is a
> > > > >        /// byte-swapping store instruction.  It byte-swaps the
> > > > >        low
> > > > >        "Type" bits of
> > > > >        /// the GPRC input, then stores it through Ptr.  Type
> > > > >        can be
> > > > >        either i16 or
> > > > >        /// i32.
> > > > > -      STBRX,
> > > > > +      STBRX = ISD::FIRST_TARGET_MEMORY_OPCODE,
> > > > > 
> > > > >        /// GPRC, CHAIN = LBRX CHAIN, Ptr, Type - This is a
> > > > >        /// byte-swapping load instruction.  It loads "Type"
> > > > >        bits,
> > > > >        byte swaps it,
> > > > > 
> > > > > Modified: llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td?rev=178438&r1=178437&r2=178438&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td (original)
> > > > > +++ llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td Sat Mar 30
> > > > > 20:58:02 2013
> > > > > @@ -452,10 +452,6 @@ def EXTSH8 : XForm_11<31, 922, (outs G8R
> > > > >  def EXTSW  : XForm_11<31, 986, (outs G8RC:$rA), (ins
> > > > >  G8RC:$rS),
> > > > >                        "extsw $rA, $rS", IntSimple,
> > > > >                        [(set i64:$rA, (sext_inreg i64:$rS,
> > > > >                        i32))]>,
> > > > >                        isPPC64;
> > > > > -/// EXTSW_32 - Just like EXTSW, but works on '32-bit'
> > > > > registers.
> > > > > -def EXTSW_32 : XForm_11<31, 986, (outs GPRC:$rA), (ins
> > > > > GPRC:$rS),
> > > > > -                      "extsw $rA, $rS", IntSimple,
> > > > > -                      [(set i32:$rA, (PPCextsw_32 i32:$rS))]>,
> > > > > isPPC64;
> > > > >  def EXTSW_32_64 : XForm_11<31, 986, (outs G8RC:$rA), (ins
> > > > >  GPRC:$rS),
> > > > >                        "extsw $rA, $rS", IntSimple,
> > > > >                        [(set i64:$rA, (sext i32:$rS))]>,
> > > > >                        isPPC64;
> > > > 
> > > > This all looks good.  Do you think we can get rid of EXTSW_32_64
> > > > as
> > > > well?  It appears to me that it's only used in two places, and in
> > > > both
> > > > places EXTSW ought to suffice.  Having two forms of extsw causes
> > > > trouble
> > > > for the assembly parser.  Just wondering if I'm missing a subtle
> > > > point
> > > > here.
> > > 
> > > Hrmm... does the pattern associated with EXTSW_32_64 match anything
> > > in practice? Maybe we could declare the sign-extension to be
> > > Promote and just define the 64-bit quantity (the use in CTRLoops
> > > could likely be handled the same way: some kind of subregister
> > > insertion and then the 64-bit version)? Would you care to
> > > investigate?
> > 
> > Sure, I'll look into it (as a low priority item).  Just wanted to
> > make
> > sure there wasn't something obvious I missed before messing with it.
> 
> Quasi-related thought, we should take a serious look at Jakob's SPARC-64 patches (from yesterday). It seems that they have a very similar 32/64-bit instruction identity issue to that in PPC, and Jakob addresses this by adding i64 as a valid type on the 32-bit register class, reusing the original 32-bit instructions (and just adding additional patterns for the 64-bit cases).

Sounds promising indeed!  I think Ulrich is off for a holiday today; we
should check first whether he's worked on something similar already as
part of the asm-parser patches.  If not I'll definitely put it on my
list of things to investigate -- have wanted to be rid of that
duplication since I started on this project. ;)

Thanks,
Bill

> 
>  -Hal
> 
> > 
> > Thanks,
> > Bill
> > 
> > > 
> > > Thanks again,
> > > Hal
> > > 
> > > > 
> > > > Thanks,
> > > > Bill
> > > > 
> > > > > @@ -786,15 +782,6 @@ def STDBRX: XForm_8<31, 660, (outs), (in
> > > > >                     "stdbrx $rS, $dst", LdStStore,
> > > > >                     [(PPCstbrx i64:$rS, xoaddr:$dst, i64)]>,
> > > > >                     isPPC64,
> > > > >                     PPC970_DGroup_Cracked;
> > > > > -
> > > > > -// STD_32/STDX_32 - Just like STD/STDX, but uses a '32-bit'
> > > > > input
> > > > > register.
> > > > > -def STD_32  : DSForm_1<62, 0, (outs), (ins GPRC:$rT,
> > > > > memrix:$dst),
> > > > > -                       "std $rT, $dst", LdStSTD,
> > > > > -                       [(PPCstd_32  i32:$rT, ixaddr:$dst)]>,
> > > > > isPPC64;
> > > > > -def STDX_32  : XForm_8<31, 149, (outs), (ins GPRC:$rT,
> > > > > memrr:$dst),
> > > > > -                       "stdx $rT, $dst", LdStSTD,
> > > > > -                       [(PPCstd_32  i32:$rT, xaddr:$dst)]>,
> > > > > isPPC64,
> > > > > -                       PPC970_DGroup_Cracked;
> > > > >  }
> > > > > 
> > > > >  // Stores with Update (pre-inc).
> > > > > 
> > > > > Modified: llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.td
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.td?rev=178438&r1=178437&r2=178438&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.td (original)
> > > > > +++ llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.td Sat Mar 30
> > > > > 20:58:02 2013
> > > > > @@ -105,10 +105,6 @@ def PPCsrl        : SDNode<"PPCISD::SRL"
> > > > >  def PPCsra        : SDNode<"PPCISD::SRA"       ,
> > > > >  SDTIntShiftOp>;
> > > > >  def PPCshl        : SDNode<"PPCISD::SHL"       ,
> > > > >  SDTIntShiftOp>;
> > > > > 
> > > > > -def PPCextsw_32   : SDNode<"PPCISD::EXTSW_32"  ,
> > > > > SDTIntUnaryOp>;
> > > > > -def PPCstd_32     : SDNode<"PPCISD::STD_32"    , SDTStore,
> > > > > -                           [SDNPHasChain, SDNPMayStore]>;
> > > > > -
> > > > >  // These are target-independent nodes, but have
> > > > >  target-specific
> > > > >  formats.
> > > > >  def callseq_start : SDNode<"ISD::CALLSEQ_START",
> > > > >  SDT_PPCCallSeqStart,
> > > > >                             [SDNPHasChain, SDNPOutGlue]>;
> > > > > 
> > > > > Modified: llvm/trunk/lib/Target/PowerPC/PPCRegisterInfo.cpp
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCRegisterInfo.cpp?rev=178438&r1=178437&r2=178438&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/lib/Target/PowerPC/PPCRegisterInfo.cpp
> > > > > (original)
> > > > > +++ llvm/trunk/lib/Target/PowerPC/PPCRegisterInfo.cpp Sat Mar
> > > > > 30
> > > > > 20:58:02 2013
> > > > > @@ -68,7 +68,7 @@ PPCRegisterInfo::PPCRegisterInfo(const P
> > > > >    ImmToIdxMap[PPC::LHZ8] = PPC::LHZX8; ImmToIdxMap[PPC::LWZ8]
> > > > >    =
> > > > >    PPC::LWZX8;
> > > > >    ImmToIdxMap[PPC::STB8] = PPC::STBX8; ImmToIdxMap[PPC::STH8]
> > > > >    =
> > > > >    PPC::STHX8;
> > > > >    ImmToIdxMap[PPC::STW8] = PPC::STWX8; ImmToIdxMap[PPC::STDU]
> > > > >    =
> > > > >    PPC::STDUX;
> > > > > -  ImmToIdxMap[PPC::ADDI8] = PPC::ADD8;
> > > > > ImmToIdxMap[PPC::STD_32] =
> > > > > PPC::STDX_32;
> > > > > +  ImmToIdxMap[PPC::ADDI8] = PPC::ADD8;
> > > > >  }
> > > > > 
> > > > >  /// getPointerRegClass - Return the register class to use to
> > > > >  hold
> > > > >  pointers.
> > > > > @@ -522,7 +522,6 @@ PPCRegisterInfo::eliminateFrameIndex(Mac
> > > > >    case PPC::LWA:
> > > > >    case PPC::LD:
> > > > >    case PPC::STD:
> > > > > -  case PPC::STD_32:
> > > > >      isIXAddr = true;
> > > > >      break;
> > > > >    }
> > > > > 
> > > > > Added: llvm/trunk/test/CodeGen/PowerPC/i32-to-float.ll
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/i32-to-float.ll?rev=178438&view=auto
> > > > > ==============================================================================
> > > > > --- llvm/trunk/test/CodeGen/PowerPC/i32-to-float.ll (added)
> > > > > +++ llvm/trunk/test/CodeGen/PowerPC/i32-to-float.ll Sat Mar 30
> > > > > 20:58:02 2013
> > > > > @@ -0,0 +1,31 @@
> > > > > +; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=g5
> > > > > |
> > > > > FileCheck %s
> > > > > +target datalayout =
> > > > > "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f128:128:128-v128:128:128-n32:64"
> > > > > +target triple = "powerpc64-unknown-linux-gnu"
> > > > > +
> > > > > +define float @foo(i32 %a) nounwind {
> > > > > +entry:
> > > > > +  %x = sitofp i32 %a to float
> > > > > +  ret float %x
> > > > > +
> > > > > +; CHECK: @foo
> > > > > +; CHECK: extsw [[REG:[0-9]+]], 3
> > > > > +; CHECK: std [[REG]],
> > > > > +; CHECK: lfd [[REG2:[0-9]+]],
> > > > > +; CHECK: fcfid [[REG3:[0-9]+]], [[REG2]]
> > > > > +; CHECK: frsp 1, [[REG3]]
> > > > > +; CHECK: blr
> > > > > +}
> > > > > +
> > > > > +define double @goo(i32 %a) nounwind {
> > > > > +entry:
> > > > > +  %x = sitofp i32 %a to double
> > > > > +  ret double %x
> > > > > +
> > > > > +; CHECK: @goo
> > > > > +; CHECK: extsw [[REG:[0-9]+]], 3
> > > > > +; CHECK: std [[REG]],
> > > > > +; CHECK: lfd [[REG2:[0-9]+]],
> > > > > +; CHECK: fcfid 1, [[REG2]]
> > > > > +; CHECK: blr
> > > > > +}
> > > > > +
> > > > > 
> > > > > 
> > > > > _______________________________________________
> > > > > llvm-commits mailing list
> > > > > llvm-commits at cs.uiuc.edu
> > > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> > 
> 





More information about the llvm-commits mailing list