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

Friedman, Eli via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 21 11:44:10 PST 2017


I would rather move the actual code to adjust the shift amount type, 
instead of just the assert.

-Eli

On 11/21/2017 9:08 AM, Liu, Yaxun (Sam) wrote:
>
> How about moving the assert to LegalizeTypes.cpp, right before the 
> change? Since that’s where regression may happen.
>
> Thanks.
>
> Sam
>
> *From:*Friedman, Eli [mailto:efriedma at codeaurora.org]
> *Sent:* Monday, November 20, 2017 9:46 PM
> *To:* Liu, Yaxun (Sam) <Yaxun.Liu at amd.com>; llvm-commits at lists.llvm.org
> *Subject:* Re: [llvm] r318727 - [AMDGPU] Fix 
> DAGTypeLegalizer::SplitInteger for shift amount type
>
> 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


-- 
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/20171121/54f80d33/attachment.html>


More information about the llvm-commits mailing list