Re: [PATCH v4 3/6] tree-object-size: Support dynamic sizes in conditions

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Dec 15, 2021 at 11:26:48PM +0530, Siddhesh Poyarekar wrote:
> > This makes me a little bit worried.  Do you compute the wholesize SSA_NAME
> > at runtime always, or only when it is really needed and known not to always
> > be equal to the size?
> > I mean, e.g. for the cases where there is just const char *p = malloc (size);
> > and the pointer is never increased size == wholesize.  For __bos it will
> > just be 2 different INTEGER_CSTs, but if it would at runtime mean we compute
> > something twice and hope we eventually find out during later passes
> > it is the same, it would be bad.
> 
> I'm emitting both size and wholesize all the time; wholesize only really
> gets used in size_for_offset and otherwise should get DCE'd.  Right now for
> __bos (and constant sizes) wholesize is unused if it is the same as size.
> 
> FOR GIMPLE_CALL, GIMPLE_NOP, etc. I return the same tree for size and
> wholesize; maybe a trivial pointer comparison (sz != wholesize) ought to get
> rid of most of the uses in size_for_offset.

Perhaps DCE can handle well when you compute something (wholesize) that isn't really
needed and VN/CSE the case where size and wholesize is equal.  I think it
would be worth looking at a few testcases.

> > > +	{
> > > +	  edge e = gimple_phi_arg_edge (obj_phi, i);
> > > +
> > > +	  /* Put the size definition before the last statement of the source
> > > +	     block of the PHI edge.  This ensures that any branches at the end
> > > +	     of the source block remain the last statement.  We are OK even if
> > > +	     the last statement is the definition of the object since it will
> > > +	     succeed any definitions that contribute to its size and the size
> > > +	     expression will succeed them too.  */
> > > +	  gimple_stmt_iterator gsi = gsi_last_bb (e->src);
> > > +	  gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING);
> > 
> > This looks problematic.  The last stmt in the bb might not exist at all,
> 
> Wouldn't the bb minimally have to contain the definition of the object whose
> size we computed?  e.g. for PHI [a(2), b(3)], wouldn't bb 2 at least have a
> statement with the definition of a?

It can e.g. contain just a PHI.

> Or wait, there could be situations where the definition is in a different
> block, e.g. bb 1, which has a single edge going on to bb 2?

> I suppose __bos-like behaviour could be a good compromise, i.e. insert a
> MAX_EXPR (or MIN_EXPR) if we can't find a suitable location to insert on
> edge.

MAX_EXPR or MIN_EXPR?  I'd have expect the __bos constant in there.
But I must say I'm right now unsure what kind of PHIs one can have on bbs
reachable from both ab/eh edges and normal edges if we can have such bbs at
all.  I guess looking at some sigjmp/longjmp or non-local or computed goto
testcases might show something, perhaps I'll have a look tomorrow.
I'm sure we can have vop PHI.

	Jakub