[llvm-commits] [PATCH, RFC] Fix problem with combining a load and shift
Evan Cheng
evan.cheng at apple.com
Fri Jan 11 16:27:52 PST 2013
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
More information about the llvm-commits
mailing list