On Mon, 27 Nov 2023 at 20:03, Richard Sandiford <richard.sandif...@arm.com> wrote: > > Joern Rennecke <joern.renne...@embecosm.com> writes: > > On 11/20/23 11:26, Richard Sandiford wrote: > >>> + /* ?!? What is the point of this adjustment to DST_MASK? */ > >>> + if (code == PLUS || code == MINUS > >>> + || code == MULT || code == ASHIFT) > >>> + dst_mask > >>> + = dst_mask ? ((2ULL << floor_log2 (dst_mask)) - 1) : 0; > >> > >> Yeah, sympathise with the ?!? here :) > > Jeff Law: > >> Inherited. Like the other bit of magic I think I'll do a test with them > >> pulled out to see if I can make something undesirable trigger. > > > > This represents the carry effect. Even if the destination only cares about > > some high order bits, you have to consider all lower order bits of the > > inputs. > > > > For ASHIFT, you could refine this in the case of a constant shift count. > > Ah, right. Think it would be worth a comment. > > But I wonder whether we should centralise all this code-specific > information into a single place. I.e. rather than having one switch to > say "PLUS is OK" or "AND is OK", and then having code-specific handling > elsewhere, we could enumerate how to handle a code.
This carry-back-propagation code is used only in that one place, so I saw no need to put it in a separate function. But if we need to add to it (handle SIGN_EXTEND, maybe handle ASHIFT better) and add lots of comments, it makes sense to put it into an inlinable function so it doesn't disrupt the flow of reading the code. Maybe something like this? /* X, with code CODE, is an operation for which safe_for_live_propagation holds true, and bits set in MASK are live in the result. Compute a make of (potentially) live bits in the non-constant inputs. In case of binop_implies_op2_fully_live (e.g. shifts), the computed mask may exclusively pertain to the first operand. */ HOST_WIDE_INT carry_backpropagate (HOST_WIDE_INT mask, enum rtx_code code, rtx x)
