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

[Liu, Yaxun (Sam) via llvm-commits]

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 223-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/20171121/0da02f03/attachment.html>