On Thu, 2020-03-12 at 15:26 -0500, Segher Boessenkool wrote: > On Thu, Mar 12, 2020 at 12:47:04PM -0600, Jeff Law wrote: > > On Thu, 2020-03-12 at 13:23 -0500, Segher Boessenkool wrote: > > > > else if (n_sets == 1 > > > > - && MEM_P (trial) > > > > + && ! CALL_P (insn) > > > > + && (MEM_P (trial) || REG_P (trial)) > > > > && MEM_P (dest) > > > > && rtx_equal_p (trial, dest) > > > > && !side_effects_p (dest) > > > > > > This adds the !CALL_P (no space btw) condition, why is that? > > Because n_sets is not valid for CALL_P insns which resulted in a failure on > > ppc. > > See find_sets_in_insn which ignores the set on the LHS of a call. So > > imagine > > if > > we had a nop register set in parallel with a (set (reg) (call ...)). We'd > > end up > > deleting the entire PARALLEL which is obviously wrong. > > Ah, I see. So this is exposed on Power by the TOC stuff, I guess? CSE > sees a TOC set parallel with a call as a no-op because it is set to the > same value (an unspec, not an unspec_volatile) that GCC can derive is > already in the TOC reg? Or is this some other case? Not entirely sure. Richard's message didn't include the precise details.
> > The change sounds fine, fwiw. > > > One could argue that find_sets_in_insn should be fixed as well. I'd be > > worried > > about fallout from that. > > Should it ignore all SETs in parallel with a call? Or what do you want > to fix there? I was thinking the other way. Make it include sets even those for the function return value. Having n_sets's meaning be different for CALL_INSNs vs other INSNs just seems like a poor implementation decision because of the inconsistency. jeff
