[PATCH] D26367: Fix DAGCombiner match

[James Molloy via Phabricator via llvm-commits]

jmolloy added a comment.

Hi,

In https://reviews.llvm.org/D26367#599175, @evstupac wrote:

> >> This LGTM but I think we should probably add tests for other targets as well.
>
> There are similar tests on ARM only. When I tried to extend them I've got the following:
>
>   ldrb r12, [lr, r12]!
>   ldrb lr, [lr, #1]
>
>
> Instead of 1 ldrh.
>
> I'm not sure what is more profitable for ARM and which -mtriple/-mcpu is better. So I'd better leave the test inserting for ARM guys.


This is not a good result. It seems fairly obvious to me that two instructions are worse than one - an LDRH would be what I would expect generated.

James



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11261
       int64_t Offset = cast<ConstantSDNode>(Ptr->getOperand(1))->getSExtValue();
-      return  BaseIndexOffset(Ptr->getOperand(0), SDValue(), Offset,
-                              IsIndexSignExt);
+      return match(Ptr->getOperand(0), DAG, Offset + PartialOffset);
     }
----------------
Why has IsIndexSignExt been removed here?


Repository:
  rL LLVM

https://reviews.llvm.org/D26367