On Mon, Jun 19, 2017 at 03:28:20PM +0100, Richard Earnshaw (lists) wrote: > > That's not what combine does: it optimistically assumes any combination > > with unknown costs is an improvement. > > So try this testcase on ARM. > > unsigned long x, y, z; > int b; > void test() > { > b = __builtin_sub_overflow (y,z, &x); > } > > > Without the patch, combine rips apart a compare and subtract insn > because it sees it as having cost zero and substitutes it with separate > compare and subtract insns.
> The combine log before the patch shows: > > allowing combination of insns 10 and 51 > original costs 0 + 8 = 0 > replacement costs 4 + 12 = 16 Yes, this is a good example of a case where your patch helps. Thanks. > So it is clearly deciding that the original costs are greater than the > replacement costs. No: it allows any combination with unknown cost (either old or new cost). See combine_validate_cost. > >> This patch addresses this problem by allowing insn_rtx_cost to ignore > >> the condition setting part of a PARALLEL iff there is exactly one > >> comparison set and one non-comparison set. If the only set operation is > >> a comparison we still use that as the basis of the insn cost. > > > > I'll test this on a zillion archs, see what the effect is. > > > > Have you considered costing general parallels as well? > > I thought about it but concluded that there's no generically correct > answer. It might be the max of all the individual sets or it might be > the sum, or it might be somewhere in between. For example on ARM the > load/store multiple operations are expressed as parallels, but their > cost will depend on how many loads/stores happen in parallel within the > hardware. > > I think we'd need a new back-end hook to handle the other cases sensibly. And in general make insn_rtx_cost do something more sane than just looking at a set_src_cost, yeah. The problem is changing any of this without regressing some targets. Of course we are in stage 1 now ;-) Segher