[PATCH] Implement more expansions of extloads

[Matt Arsenault]

> On Jan 13, 2015, at 4:03 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> 
>> From: Matt Arsenault <Matthew.Arsenault at amd.com>
>> Date: Wed, 7 Jan 2015 22:42:21 -0500
>> Subject: [PATCH 1/3] Implement new way of expanding extloads.
> 
> This mostly LGTM, with one concern; comments inline.  I'll leave the
> other patches and R600 parts to others.
> 
> -Ahmed
> 
>> Now that the source and destination types can be specified,
>> allow doing an expansion that doesn't use an EXTLOAD of the
>> result type. Try to do a legal extload to an intermediate type
>> and extend that if possible.
>> 
>> This generalizes the special case custom lowering of extloads
>> R600 has been using to work around this problem.
>> 
>> This also happens to fix a bug that would incorrectly use more
>> aligned loads than should be used.
>> ---
> 
>> diff --git a/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
>> index ce8f96b..d86c20c 100644
>> --- a/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
>> +++ b/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
>> @@ -1090,22 +1090,35 @@ void SelectionDAGLegalize::LegalizeLoadOps(SDNode *Node) {
>>       break;
>>     }
>>     case TargetLowering::Expand:
>> -      if (!TLI.isLoadExtLegal(ISD::EXTLOAD, Node->getValueType(0),
>> -                              SrcVT) && TLI.isTypeLegal(SrcVT)) {
>> -        SDValue Load = DAG.getLoad(SrcVT, dl, Chain, Ptr, LD->getMemOperand());
>> -        unsigned ExtendOp;
>> -        switch (ExtType) {
>> -        case ISD::EXTLOAD:
>> -          ExtendOp = (SrcVT.isFloatingPoint() ?
>> -                      ISD::FP_EXTEND : ISD::ANY_EXTEND);
>> +      if (!TLI.isLoadExtLegal(ISD::EXTLOAD, Node->getValueType(0), SrcVT)) {
>> +        // Figure out the type to load to before extending.
>> +        EVT LoadVT = SrcVT;
>> +
>> +        if (TLI.isLoadExtLegal(ISD::EXTLOAD, Node->getValueType(0),
>> +                               SrcVT) && TLI.isTypeLegal(SrcVT)) {
> 
> This contradicts the outer if, no? Should it be just "TLI.isTypeLegal(SrcVT)" ?
> If so, since this doesn't break, I'm guessing it's no longer tested?
> (not that I know where one would do that.)
> 
>> +          SDValue Load = DAG.getLoad(SrcVT, dl, Chain, Ptr, LD->getMemOperand());
> 
> Nit: 80 cols (same later)
> 
>> +          unsigned ExtendOp
>> +            = ISD::getExtForLoadExtType(SrcVT.isFloatingPoint(), ExtType);
> 
> Nit: clang-format? The "=" would go in the first line (same later).
> 
>> +          Value = DAG.getNode(ExtendOp, dl, Node->getValueType(0), Load);
>> +          Chain = Load.getValue(1);
>> +          break;
>> +        }
>> +
>> +        // If the source type is not legal, see if there is a legal extload to
>> +        // an intermediate type that we can then extend further.
>> +        LoadVT = TLI.getRegisterType(SrcVT.getSimpleVT());
>> +        if (LoadVT == SrcVT || TLI.isLoadExtLegal(ExtType, LoadVT, SrcVT)) {
>> +          ISD::LoadExtType MidExtTy
> 
> Nit: how about matching ExtType, and naming this MidExtType
> 
>> +            = (LoadVT == SrcVT) ? ISD::NON_EXTLOAD : ExtType;
>> +
>> +          SDValue Load = DAG.getExtLoad(MidExtTy, dl, LoadVT, Chain,
>> +                                        Ptr, SrcVT, LD->getMemOperand());
>> +          unsigned ExtendOp
>> +            = ISD::getExtForLoadExtType(SrcVT.isFloatingPoint(), ExtType);
>> +          Value = DAG.getNode(ExtendOp, dl, Node->getValueType(0), Load);
>> +          Chain = Load.getValue(1);
> 
> Nit: both blocks are pretty much the same, with a different MidExtTy.
> I'm not sure it would be more readable, but maybe there's a way to
> avoid the duplication?
> 

I figured out why deleting the first block works. The TLI.getRegisterType(SrcVT) == SrcVT is (almost?) always equivalent to isTypeLegal, so I’ve eliminated the first block.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implement-new-way-of-expanding-extloads.patch
Type: application/octet-stream
Size: 62139 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150113/d0c4f162/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-R600-SI-Fix-bad-code-with-unaligned-byte-vector-load.patch
Type: application/octet-stream
Size: 5703 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150113/d0c4f162/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-R600-SI-Remove-some-redudant-load-testcases.patch
Type: application/octet-stream
Size: 5825 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150113/d0c4f162/attachment-0002.obj>
-------------- next part --------------