[PATCH] R600/SI: Try to use scalar BFE.

Tom Stellard tom at stellard.net
Wed Apr 2 10:04:15 PDT 2014


On Tue, Apr 01, 2014 at 11:33:07AM -0700, Matt Arsenault wrote:
> Use scalar BFE with constant shift and offset when possible.
> This is complicated by the fact that the scalar version packs
> the two operands of the vector version into one.
> 

LGTM.

> 
> http://llvm-reviews.chandlerc.com/D3249
> 
> Files:
>   lib/Target/R600/AMDGPUISelDAGToDAG.cpp
>   lib/Target/R600/SIInstrInfo.cpp
>   test/CodeGen/R600/sext-in-reg.ll

> Index: lib/Target/R600/AMDGPUISelDAGToDAG.cpp
> ===================================================================
> --- lib/Target/R600/AMDGPUISelDAGToDAG.cpp
> +++ lib/Target/R600/AMDGPUISelDAGToDAG.cpp
> @@ -198,13 +198,14 @@
>      N->setNodeId(-1);
>      return NULL;   // Already selected.
>    }
> +
> +  const AMDGPUSubtarget &ST = TM.getSubtarget<AMDGPUSubtarget>();
>    switch (Opc) {
>    default: break;
>    // We are selecting i64 ADD here instead of custom lower it during
>    // DAG legalization, so we can fold some i64 ADDs used for address
>    // calculation into the LOAD and STORE instructions.
>    case ISD::ADD: {
> -    const AMDGPUSubtarget &ST = TM.getSubtarget<AMDGPUSubtarget>();
>      if (N->getValueType(0) != MVT::i64 ||
>          ST.getGeneration() < AMDGPUSubtarget::SOUTHERN_ISLANDS)
>        break;
> @@ -250,7 +251,6 @@
>    }
>    case ISD::BUILD_VECTOR: {
>      unsigned RegClassID;
> -    const AMDGPUSubtarget &ST = TM.getSubtarget<AMDGPUSubtarget>();
>      const AMDGPURegisterInfo *TRI =
>                     static_cast<const AMDGPURegisterInfo*>(TM.getRegisterInfo());
>      const SIRegisterInfo *SIRI =
> @@ -337,7 +337,6 @@
>    }
>    case ISD::BUILD_PAIR: {
>      SDValue RC, SubReg0, SubReg1;
> -    const AMDGPUSubtarget &ST = TM.getSubtarget<AMDGPUSubtarget>();
>      if (ST.getGeneration() <= AMDGPUSubtarget::NORTHERN_ISLANDS) {
>        break;
>      }
> @@ -358,7 +357,6 @@
>                                    SDLoc(N), N->getValueType(0), Ops);
>    }
>    case AMDGPUISD::REGISTER_LOAD: {
> -    const AMDGPUSubtarget &ST = TM.getSubtarget<AMDGPUSubtarget>();
>      if (ST.getGeneration() <= AMDGPUSubtarget::NORTHERN_ISLANDS)
>        break;
>      SDValue Addr, Offset;
> @@ -375,7 +373,6 @@
>                                    Ops);
>    }
>    case AMDGPUISD::REGISTER_STORE: {
> -    const AMDGPUSubtarget &ST = TM.getSubtarget<AMDGPUSubtarget>();
>      if (ST.getGeneration() <= AMDGPUSubtarget::NORTHERN_ISLANDS)
>        break;
>      SDValue Addr, Offset;
> @@ -391,6 +388,47 @@
>                                          CurDAG->getVTList(MVT::Other),
>                                          Ops);
>    }
> +
> +  case AMDGPUISD::BFE_I32:
> +  case AMDGPUISD::BFE_U32: {
> +    if (ST.getGeneration() < AMDGPUSubtarget::SOUTHERN_ISLANDS)
> +      break;
> +
> +    // There is a scalar version available, but unlike the vector version which
> +    // has a separate operand for the offset and width, the scalar version packs
> +    // the width and offset into a single operand. Try to move to the scalar
> +    // version if the offsets are constant, so that we can try to keep extended
> +    // loads of kernel arguments in SGPRs.
> +
> +    // TODO: Technically we could try to pattern match scalar bitshifts of
> +    // dynamic values, but it's probably not useful.
> +    ConstantSDNode *Offset = dyn_cast<ConstantSDNode>(N->getOperand(1));
> +    if (!Offset)
> +      break;
> +
> +    ConstantSDNode *Width = dyn_cast<ConstantSDNode>(N->getOperand(2));
> +    if (!Width)
> +      break;
> +
> +    bool Signed = Opc == AMDGPUISD::BFE_I32;
> +
> +    // Transformation function, pack the offset and width of a BFE into
> +    // the format expected by the S_BFE_I32 / S_BFE_U32. In the second
> +    // source, bits [5:0] contain the offset and bits [22:16] the width.
> +
> +    uint32_t OffsetVal = Offset->getZExtValue();
> +    uint32_t WidthVal = Width->getZExtValue();
> +
> +    uint32_t PackedVal = OffsetVal | WidthVal << 16;
> +
> +    SDValue PackedOffsetWidth = CurDAG->getTargetConstant(PackedVal, MVT::i32);
> +    return CurDAG->getMachineNode(Signed ? AMDGPU::S_BFE_I32 : AMDGPU::S_BFE_U32,
> +                                  SDLoc(N),
> +                                  MVT::i32,
> +                                  N->getOperand(0),
> +                                  PackedOffsetWidth);
> +
> +  }
>    }
>    return SelectCode(N);
>  }
> Index: lib/Target/R600/SIInstrInfo.cpp
> ===================================================================
> --- lib/Target/R600/SIInstrInfo.cpp
> +++ lib/Target/R600/SIInstrInfo.cpp
> @@ -538,6 +538,8 @@
>    case AMDGPU::S_LSHR_B64: return AMDGPU::V_LSHR_B64;
>    case AMDGPU::S_SEXT_I32_I8: return AMDGPU::V_BFE_I32;
>    case AMDGPU::S_SEXT_I32_I16: return AMDGPU::V_BFE_I32;
> +  case AMDGPU::S_BFE_U32: return AMDGPU::V_BFE_U32;
> +  case AMDGPU::S_BFE_I32: return AMDGPU::V_BFE_I32;
>    }
>  }
>  
> @@ -989,6 +991,22 @@
>  
>      addDescImplicitUseDef(NewDesc, Inst);
>  
> +    if (Opcode == AMDGPU::S_BFE_I32 || Opcode == AMDGPU::S_BFE_U32) {
> +      const MachineOperand &OffsetWidthOp = Inst->getOperand(2);
> +      // If we need to move this to VGPRs, we need to unpack the second operand
> +      // back into the 2 separate ones for bit offset and width.
> +      assert(OffsetWidthOp.isImm() &&
> +             "Scalar BFE is only implemented for constant width and offset");
> +      uint32_t Imm = OffsetWidthOp.getImm();
> +
> +      uint32_t Offset = Imm & 0x3f; // Extract bits [5:0].
> +      uint32_t BitWidth = (Imm & 0x7f0000) >> 16; // Extract bits [22:16].
> +
> +      Inst->RemoveOperand(2); // Remove old immediate.
> +      Inst->addOperand(MachineOperand::CreateImm(Offset));
> +      Inst->addOperand(MachineOperand::CreateImm(BitWidth));
> +    }
> +
>      legalizeOperands(Inst);
>  
>      // Update the destination register class.
> Index: test/CodeGen/R600/sext-in-reg.ll
> ===================================================================
> --- test/CodeGen/R600/sext-in-reg.ll
> +++ test/CodeGen/R600/sext-in-reg.ll
> @@ -6,7 +6,8 @@
>  
>  ; FUNC-LABEL: @sext_in_reg_i1_i32
>  ; SI: S_LOAD_DWORD [[ARG:s[0-9]+]],
> -; SI: V_BFE_I32 [[EXTRACT:v[0-9]+]], [[ARG]], 0, 1
> +; SI: S_BFE_I32 [[SEXTRACT:s[0-9]+]], [[ARG]], 65536
> +; SI: V_MOV_B32_e32 [[EXTRACT:v[0-9]+]], [[SEXTRACT]]
>  ; SI: BUFFER_STORE_DWORD [[EXTRACT]],
>  
>  ; EG: BFE_INT
> @@ -110,8 +111,8 @@
>  
>  ; This is broken on Evergreen for some reason related to the <1 x i64> kernel arguments.
>  ; XFUNC-LABEL: @sext_in_reg_i8_to_v1i64
> -; XSI: V_BFE_I32 {{v[0-9]+}}, {{s[0-9]+}}, 0, 8
> -; XSI: V_ASHRREV_I32_e32 {{v[0-9]+}}, 31,
> +; XSI: S_BFE_I32 [[EXTRACT:s[0-9]+]], {{s[0-9]+}}, 524288
> +; XSI: S_ASHR_I32 {{v[0-9]+}}, [[EXTRACT]], 31
>  ; XSI: BUFFER_STORE_DWORD
>  ; XEG: BFE_INT
>  ; XEG: ASHR
> @@ -152,8 +153,8 @@
>  
>  
>  ; FUNC-LABEL: @sext_in_reg_v2i1_to_v2i32
> -; SI: V_BFE_I32 {{v[0-9]+}}, {{s[0-9]+}}, 0, 1
> -; SI: V_BFE_I32 {{v[0-9]+}}, {{s[0-9]+}}, 0, 1
> +; SI: S_BFE_I32 {{s[0-9]+}}, {{s[0-9]+}}, 65536
> +; SI: S_BFE_I32 {{s[0-9]+}}, {{s[0-9]+}}, 65536
>  ; SI: BUFFER_STORE_DWORDX2
>  ; EG: BFE
>  ; EG: BFE
> @@ -166,10 +167,10 @@
>  }
>  
>  ; FUNC-LABEL: @sext_in_reg_v4i1_to_v4i32
> -; SI: V_BFE_I32 {{v[0-9]+}}, {{s[0-9]+}}, 0, 1
> -; SI: V_BFE_I32 {{v[0-9]+}}, {{s[0-9]+}}, 0, 1
> -; SI: V_BFE_I32 {{v[0-9]+}}, {{s[0-9]+}}, 0, 1
> -; SI: V_BFE_I32 {{v[0-9]+}}, {{s[0-9]+}}, 0, 1
> +; SI: S_BFE_I32 {{s[0-9]+}}, {{s[0-9]+}}, 65536
> +; SI: S_BFE_I32 {{s[0-9]+}}, {{s[0-9]+}}, 65536
> +; SI: S_BFE_I32 {{s[0-9]+}}, {{s[0-9]+}}, 65536
> +; SI: S_BFE_I32 {{s[0-9]+}}, {{s[0-9]+}}, 65536
>  ; SI: BUFFER_STORE_DWORDX4
>  
>  ; EG: BFE
> @@ -257,6 +258,34 @@
>    ret void
>  }
>  
> +; FUNC-LABEL: @vgpr_sext_in_reg_v4i8_to_v4i32
> +; SI: V_BFE_I32 [[EXTRACT:v[0-9]+]], {{v[0-9]+}}, 0, 8
> +; SI: V_BFE_I32 [[EXTRACT:v[0-9]+]], {{v[0-9]+}}, 0, 8
> +; SI: V_BFE_I32 [[EXTRACT:v[0-9]+]], {{v[0-9]+}}, 0, 8
> +; SI: V_BFE_I32 [[EXTRACT:v[0-9]+]], {{v[0-9]+}}, 0, 8
> +define void @vgpr_sext_in_reg_v4i8_to_v4i32(<4 x i32> addrspace(1)* %out, <4 x i32> addrspace(1)* %a, <4 x i32> addrspace(1)* %b) nounwind {
> +  %loada = load <4 x i32> addrspace(1)* %a, align 16
> +  %loadb = load <4 x i32> addrspace(1)* %b, align 16
> +  %c = add <4 x i32> %loada, %loadb ; add to prevent folding into extload
> +  %shl = shl <4 x i32> %c, <i32 24, i32 24, i32 24, i32 24>
> +  %ashr = ashr <4 x i32> %shl, <i32 24, i32 24, i32 24, i32 24>
> +  store <4 x i32> %ashr, <4 x i32> addrspace(1)* %out, align 8
> +  ret void
> +}
> +
> +; FUNC-LABEL: @vgpr_sext_in_reg_v4i16_to_v4i32
> +; SI: V_BFE_I32 [[EXTRACT:v[0-9]+]], {{v[0-9]+}}, 0, 16
> +; SI: V_BFE_I32 [[EXTRACT:v[0-9]+]], {{v[0-9]+}}, 0, 16
> +define void @vgpr_sext_in_reg_v4i16_to_v4i32(<4 x i32> addrspace(1)* %out, <4 x i32> addrspace(1)* %a, <4 x i32> addrspace(1)* %b) nounwind {
> +  %loada = load <4 x i32> addrspace(1)* %a, align 16
> +  %loadb = load <4 x i32> addrspace(1)* %b, align 16
> +  %c = add <4 x i32> %loada, %loadb ; add to prevent folding into extload
> +  %shl = shl <4 x i32> %c, <i32 16, i32 16, i32 16, i32 16>
> +  %ashr = ashr <4 x i32> %shl, <i32 16, i32 16, i32 16, i32 16>
> +  store <4 x i32> %ashr, <4 x i32> addrspace(1)* %out, align 8
> +  ret void
> +}
> +
>  ; FIXME: The BFE should really be eliminated. I think it should happen
>  ; when computeMaskedBitsForTargetNode is implemented for imax.
>  

> _______________________________________________
> 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