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

[Evan Cheng]

LGTM
On Jan 13, 2013, at 8:16 AM, Bill Schmidt <wschmidt at linux.vnet.ibm.com> wrote:

> 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
>> 
> <combine-2013-01-13.patch>