[PATCH] Implement more expansions of extloads

[Ahmed Bougacha]

> 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?

>            break;
> -        case ISD::SEXTLOAD: ExtendOp = ISD::SIGN_EXTEND; break;
> -        case ISD::ZEXTLOAD: ExtendOp = ISD::ZERO_EXTEND; break;
> -        default: llvm_unreachable("Unexpected extend load type!");
>          }
> -        Value = DAG.getNode(ExtendOp, dl, Node->getValueType(0), Load);
> -        Chain = Load.getValue(1);
> -        break;
>        }
>
>        assert(!SrcVT.isVector() &&