[PATCH] Implement more expansions of extloads

[Ahmed Bougacha]

On Wed, Jan 14, 2015 at 2:01 AM, Matt Arsenault <arsenm2 at gmail.com> wrote:
>
>> 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.

Ah yes, that sounds reasonable.  I also would be very surprised if
there were cases were that's not equivalent; maybe you could even add
an assert before the inner if to make sure that's the case.  With that
and a little more explicit comment for the legal/non_extload case, the
legalization part LGTM.

Thanks,

-Ahmed