[AMDGPU] Fix DAGTypeLegalizer::SplitInteger for shift amount type

[yaxunl]

Summary

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.

[arsenm]

SplitInteger,
where default pointer type is used for the constant 16.

This is wrong. This should not be using getIntPtrConstant. For AMDGPU the shift type is always 32. If somewhere is creating a shift mask with something other than getScalarShiftAmountTy, that is a bug there.

[yaxunl]

In D40148#928236, @arsenm wrote:

SplitInteger,
where default pointer type is used for the constant 16.

This is wrong. This should not be using getIntPtrConstant. For AMDGPU the shift type is always 32. If somewhere is creating a shift mask with something other than getScalarShiftAmountTy, that is a bug there.

I see. Will fix SplitInteger to use getScalarShiftAmountTy.

[yaxunl]

Revised by Matt's comments.

[arsenm]

LGTM, but adding reviewers for x86 change

[RKSimon]

LGTM with one minor

lib/Target/X86/X86ISelLowering.h

669 ↗

(On Diff #123352)

Please can you put in an assert to check that the return type is big enough to hold the max shift value for VT.

[yaxunl]

lib/Target/X86/X86ISelLowering.h

669 ↗

(On Diff #123352)

Will do. Thanks.

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp

 /// Return the lower LoVT bits of Op in Lo and the upper HiVT bits in Hi.
 void DAGTypeLegalizer::SplitInteger(SDValue Op,
                                     EVT LoVT, EVT HiVT,
                                     SDValue &Lo, SDValue &Hi) {
   SDLoc dl(Op);
   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,
+                                  TLI.getScalarShiftAmountTy(
+                                      DAG.getDataLayout(), Op.getValueType())));
   Hi = DAG.getNode(ISD::TRUNCATE, dl, HiVT, Hi);
 }
 
 /// Return the lower and upper halves of Op's bits in a value type half the
 /// size of Op's.
 void DAGTypeLegalizer::SplitInteger(SDValue Op,
                                     SDValue &Lo, SDValue &Hi) {
   EVT HalfVT =

llvm/trunk/lib/Target/X86/X86ISelLowering.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_X86_X86ISELLOWERING_H
 #define LLVM_LIB_TARGET_X86_X86ISELLOWERING_H
 
 #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 {
   class X86Subtarget;
   class X86TargetMachine;
 
   namespace X86ISD {
     // X86 Specific DAG Nodes
explicit X86TargetLowering(const X86TargetMachine &TM,
                                const X86Subtarget &STI);
 
     unsigned getJumpTableEncoding() const override;
     bool useSoftFloat() const override;
 
     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;
+      assert((VT.getSizeInBits() + 1) / 2 < (1U << T.getSizeInBits()) &&
+             "Scalar shift amount type too small");
+      return T;
     }
 
     const MCExpr *
     LowerCustomJumpTableEntry(const MachineJumpTableInfo *MJTI,
                               const MachineBasicBlock *MBB, unsigned uid,
                               MCContext &Ctx) const override;
 
     /// Returns relocation base for the given PIC jumptable.

llvm/trunk/test/CodeGen/AMDGPU/insert_vector_elt.v2i16.ll

-; RUN: llc -verify-machineinstrs -march=amdgcn -mcpu=gfx900 -enable-amdgpu-aa=0 -mattr=+flat-for-global,-fp64-fp16-denormals < %s | FileCheck -enable-var-scope -check-prefix=GCN -check-prefix=GFX9 -check-prefix=GFX89 %s
-; RUN: llc -verify-machineinstrs -march=amdgcn -mcpu=fiji -enable-amdgpu-aa=0 -mattr=+flat-for-global < %s | FileCheck -enable-var-scope -check-prefix=GCN -check-prefix=CIVI -check-prefix=VI -check-prefix=GFX89 %s
-; RUN: llc -verify-machineinstrs -march=amdgcn -mcpu=hawaii -enable-amdgpu-aa=0 -mattr=+flat-for-global < %s | FileCheck -enable-var-scope -check-prefix=GCN -check-prefix=CIVI -check-prefix=CI %s
+; RUN: llc -verify-machineinstrs -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=gfx900 -enable-amdgpu-aa=0 -mattr=+flat-for-global,-fp64-fp16-denormals < %s | FileCheck -check-prefix=GCN -check-prefix=GFX9 -check-prefix=GFX89 %s
+; RUN: llc -verify-machineinstrs -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=fiji -enable-amdgpu-aa=0 -mattr=+flat-for-global < %s | FileCheck -check-prefix=GCN -check-prefix=CIVI -check-prefix=VI -check-prefix=GFX89 %s
+; RUN: llc -verify-machineinstrs -march=amdgcn -mtriple=amdgcn---amdgiz -mcpu=hawaii -enable-amdgpu-aa=0 -mattr=+flat-for-global < %s | FileCheck -check-prefix=GCN -check-prefix=CIVI -check-prefix=CI %s
 
 ; GCN-LABEL: {{^}}s_insertelement_v2i16_0:
 ; GCN: s_load_dword [[VEC:s[0-9]+]]
 
 ; CIVI: s_and_b32 [[ELT1:s[0-9]+]], [[VEC]], 0xffff0000{{$}}
 ; CIVI: s_or_b32 s{{[0-9]+}}, [[ELT1]], 0x3e7{{$}}
 
 ; GFX9-NOT: lshr