<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 11/20/2017 6:29 PM, Yaxun Liu via
llvm-commits wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20171121022954.6B62B83757@lists.llvm.org">
<pre wrap="">Author: yaxunl
Date: Mon Nov 20 18:29:54 2017
New Revision: 318727
URL: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=318727&view=rev">http://llvm.org/viewvc/llvm-project?rev=318727&view=rev</a>
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: <a class="moz-txt-link-freetext" href="https://reviews.llvm.org/D40148">https://reviews.llvm.org/D40148</a>
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: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp?rev=318727&r1=318726&r2=318727&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp?rev=318727&r1=318726&r2=318727&view=diff</a>
==============================================================================
--- 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: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.h?rev=318727&r1=318726&r2=318727&view=diff">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.h?rev=318727&r1=318726&r2=318727&view=diff</a>
==============================================================================
--- 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;</pre>
</blockquote>
<br>
From LangRef, the description of integer types: "Any bit width from
1 bit to 2<sup>23</sup>-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.)<br>
<br>
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().<br
class="Apple-interchange-newline">
<p>-Eli<br>
</p>
<pre class="moz-signature" cols="72">--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project</pre>
</body>
</html>