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

[Bill Schmidt]

On Fri, 2013-01-11 at 16:27 -0800, Evan Cheng wrote:
> 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();

OK, sure.  Modified patch attached.  Thanks for the review!

Bill

> 
> 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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: combine-2013-01-13.patch
Type: text/x-patch
Size: 3828 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130113/327aaed6/attachment.bin>