[llvm] r318727 - [AMDGPU] Fix DAGTypeLegalizer::SplitInteger for shift amount type

[Friedman, Eli via llvm-commits]

On 11/20/2017 6:29 PM, Yaxun Liu via llvm-commits wrote:
> Author: yaxunl
> Date: Mon Nov 20 18:29:54 2017
> New Revision: 318727
>
> URL: http://llvm.org/viewvc/llvm-project?rev=318727&view=rev
> Log:
> [AMDGPU] Fix DAGTypeLegalizer::SplitInteger for shift amount type
>
> DAGTypeLegalizer::SplitInteger uses default pointer size as shift amount constant type,
> which causes less performant ISA in amdgcn---amdgiz target since the default pointer
> type is i64 whereas the desired shift amount type is i32.
>
> This patch fixes that by using TLI.getScalarShiftAmountTy in DAGTypeLegalizer::SplitInteger.
>
> The X86 change is necessary since splitting i512 requires shifting amount of 256, which
> cannot be held by i8.
>
> Differential Revision: https://reviews.llvm.org/D40148
>
> Modified:
>      llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
>      llvm/trunk/lib/Target/X86/X86ISelLowering.h
>      llvm/trunk/test/CodeGen/AMDGPU/insert_vector_elt.v2i16.ll
>
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp?rev=318727&r1=318726&r2=318727&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp Mon Nov 20 18:29:54 2017
> @@ -1172,9 +1172,11 @@ void DAGTypeLegalizer::SplitInteger(SDVa
>     assert(LoVT.getSizeInBits() + HiVT.getSizeInBits() ==
>            Op.getValueSizeInBits() && "Invalid integer splitting!");
>     Lo = DAG.getNode(ISD::TRUNCATE, dl, LoVT, Op);
> -  Hi = DAG.getNode(ISD::SRL, dl, Op.getValueType(), Op,
> -                   DAG.getConstant(LoVT.getSizeInBits(), dl,
> -                                   TLI.getPointerTy(DAG.getDataLayout())));
> +  Hi =
> +      DAG.getNode(ISD::SRL, dl, Op.getValueType(), Op,
> +                  DAG.getConstant(LoVT.getSizeInBits(), dl,
> +                                  TLI.getScalarShiftAmountTy(
> +                                      DAG.getDataLayout(), Op.getValueType())));
>     Hi = DAG.getNode(ISD::TRUNCATE, dl, HiVT, Hi);
>   }
>   
>
> Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.h?rev=318727&r1=318726&r2=318727&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86ISelLowering.h (original)
> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.h Mon Nov 20 18:29:54 2017
> @@ -18,6 +18,7 @@
>   #include "llvm/CodeGen/CallingConvLower.h"
>   #include "llvm/CodeGen/SelectionDAG.h"
>   #include "llvm/CodeGen/TargetLowering.h"
> +#include "llvm/Support/MathExtras.h"
>   #include "llvm/Target/TargetOptions.h"
>   
>   namespace llvm {
> @@ -664,8 +665,14 @@ namespace llvm {
>       void markLibCallAttributes(MachineFunction *MF, unsigned CC,
>                                  ArgListTy &Args) const override;
>   
> -    MVT getScalarShiftAmountTy(const DataLayout &, EVT) const override {
> -      return MVT::i8;
> +    // For i512, DAGTypeLegalizer::SplitInteger needs a shift amount 256,
> +    // which cannot be held by i8, therefore use i16 instead. In all the
> +    // other situations i8 is sufficient.
> +    MVT getScalarShiftAmountTy(const DataLayout &, EVT VT) const override {
> +      MVT T = VT.getSizeInBits() >= 512 ? MVT::i16 : MVT::i8;

 From LangRef, the description of integer types: "Any bit width from 1 
bit to 2^23 -1 (about 8 million) can be specified."  You need at least 
an i32 to hold every possible shift amount.  (Granted, the issue is 
unlikely to come up in practice.)

AVR also has a getScalarShiftAmountTy() which returns i8.  It would be 
better to add a check somewhere to the target-independent code to avoid 
this sort of issue rather than make each target add this check to its 
own getScalarShiftAmountTy().

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171120/902e1d70/attachment.html>