[llvm-commits] [PATCH, RFC] Fix problem with combining a load and shift

[Evan Cheng]

One request:

+  // For the transform to be legal, the load must produce only two values
+  // (the value loaded and the chain).  Don't transform a pre-increment
+  // load, for example, which produces an extra value.  Otherwise the 
+  // transformation is not equivalent, and the downstream logic to replace
+  // uses gets things wrong.
+  if (cast<LoadSDNode>(N0)->getNumValues() > 2)
+    return SDValue();
+
   LoadSDNode *LN0 = cast<LoadSDNode>(N0);

Could you move the check for the following line to avoid the unnecessary cast. While you are at it, please remove the unnecessary cast just before as well:
  // Verify that we are actually reducing a load width here.
  if (cast<LoadSDNode>(N0)->getMemoryVT().getSizeInBits() < EVTBits) 
    return SDValue();

Thanks,

Evan


On Jan 9, 2013, at 10:13 AM, Bill Schmidt <wschmidt at linux.vnet.ibm.com> wrote:

> This patch addresses an incorrect transformation in the DAG combiner.
> 
> The included test case is derived from one of the GCC compatibility tests.
> The problem arises after the selection DAG has been converted to type-legalized
> form.  The combiner first sees a 64-bit load that can be converted into a
> pre-increment form.  The original load feeds into a SRL that isolates the
> upper 32 bits of the loaded doubleword.  This looks like an opportunity for
> DAGCombiner::ReduceLoadWidth() to replace the 64-bit load with a 32-bit load.
> 
> However, this transformation is not valid, as the replacement load is not
> a pre-increment load.  The pre-increment load produces an extra result,
> which feeds a subsequent add instruction.  The replacement load only has
> one result value, and this value is propagated to all uses of the pre-
> increment load, including the add.  Because the add is looking for the
> second result value as its operand, it ends up attempting to add a constant
> to a token chain, resulting in a crash.
> 
> So the patch simply disables this transformation for any load with more than
> two result values.
> 
> I've tentatively placed the new test case in CodeGen/PowerPC for want of
> a better location.  Since the IR has been type-legalized for the target
> when the bug occurs, the bug may occur only on certain targets.  The test
> just checks whether the code compiles successfully, so it is appropriate
> to execute on all targets.
> 
> Any concerns with this approach?  Is this ok for mainline?
> 
> Thanks!
> 
> Bill
> <combine-2013-01-09.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits