[PATCH] Implement more expansions of extloads

[Matt Arsenault]

On 01/13/2015 05:26 PM, Ahmed Bougacha wrote:
> 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
r225925-225927