This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: CFG transparent inlining


> On Fri, Apr 29, 2005 at 03:42:01PM +0200, Jan Hubicka wrote:
> > !       /* After inlining function marked NOTHROW into MUST_NOT_THROW region
> > ! 	 with throwing instructions we may end up with missing EH landing
> > ! 	 pad for the exceptions.  It seems safe to ignore them (by setting
> > ! 	 NOTHOROW flag on the function user promised this scenario to not
> > ! 	 happen).  See filter2.C testcase.  */
> > !       if (XEXP (i, 0))
> > ! 	make_label_edge (edge_cache, src, XEXP (i, 0),
> > ! 			 EDGE_ABNORMAL | EDGE_EH | is_call);
> 
> You're going to have to give some justification for this.  I can't think
> of why you would have to be taking care of this here, rather than up at
> the tree level.

This code should be obsolette now and I ment to kill it before sending
the patch but apparently screwed up.  It should be replaced by ....
> > *************** reachable_next_level (struct eh_region *
> > ! 	 Be conservative before inlining since new subregions may be
> > ! 	 introduced.  fixup_cfg pass takes care of removing unnecesary
> > ! 	 EH edges later on.  */
> > !       if ((info && info->saw_any_handlers) || !cfun->after_inlining)
> 
> I don't understand.  With reachable_next_level, you search up, not down.
> Inlining adds *sub*regions, meaning things that are below what currently
> exists.  At no point are we adding regions in the middle of the tree; 
> only at the leaves.
> 
> If this is supposed to be solving a real problem, then I think you're
> solving it at the wrong place.

... this...
OK the problem is, as I understnad, the fact htat if you have exception
caught by MUST_NOT_THROW region and function call is in a way, you don't
need landing pad (that would call terminate) because runtime does that
for you while unwinding the stack.
On the other hand if you catch exception that was thrown by the function
itself, you need the landing pad.
So before inlining we must be cureful to not elliminate the landing pads
that looks unnecesary and die later on.

Perhaps I am screwed up.  My reading is that reachable-next_level goes
up but it starts on the bottom.  So if we start in the duplicated EH of
some inlined region we may arrange saw_any_handlers to be true in a
places it would never be true otherwise.

> > + duplicate_stmt_eh_region_mapping (struct function *ifun, 
> > + 				  struct function *cfun,
> 
> Please don't use parameters of the same name as a well-known global.

I will fix that and other the ugly bits I overlooked (sorry, I went to
tree-inline itself several times but some of the separate bits felt off
the shelf in my cleanupp attempts

> 
> > !   /* Exception region the inlined call like in.  */
> 
> "like"?
lie...
> 
> > + 		 get_eh_last_region_number (DECL_STRUCT_FUNCTION (id->caller))
> > + 		 - get_eh_last_region_number (id->callee_cfun)
> > + 		 + TREE_INT_CST_LOW (TREE_OPERAND (*tp, 0)));
> 
> It would be nice if this operation were abstracted.

I was digging bit into history (the code is not mine, but Stuart's I
assume) and old inliner had equivalent construct, just kept arround
the offset in a variable, perhaps I can do the same...
> 
> > +       duplicate_eh_regions (cfun_to_copy, (duplicate_eh_regions_map)remap_decl,
> 
> Why this cast?  Casting functions is Bad.

the remap_decl is taking inline data as it's argument, while it don't
seem to be very seweet to export it for the callback.  I can either move
the struct inline_data; into header and avoid casts or write wrapper...
> > ! copy_res_decl_for_inlining (tree res, tree fn, tree caller,
> > !                             void *dm ATTRIBUTE_UNUSED,
> > !                             int *ndp ATTRIBUTE_UNUSED,
> > !                             tree return_slot_addr ATTRIBUTE_UNUSED)
> 
> Unused parameters in a new function?

Uhm.  It used to be a hook and I moved it off without looking into
contents curefully.
> 
> > !   /* All passes needed to lower the function into shape optimizers can operate
> > !      on.  We need these to be separate from local optimization because C++ needs
> > !      to go into lowered form earlier to perfrom template instantiation.  */
> 
> Hmm?  Care to explain?

Hmm, we used to lower early in C++ frontend before clonning, but this no
longer happens, so it is useless.  I will re-unify those passes.
> 
> That's about all I could see on this pass through.  But much was 
> glossed over.
> 
> I'd like to not see this again as a 3500 line patch.  I know there are 
> parts of it that are independent bug fixes; lets get those in first.
> I know there are parts that are just infrastructure, and could go in
> without being used.

OK, I will try harder to break it up more...
Definitly the EH duplication infrastructure can go in separately unused.
Not sure how much else I can break up except for trivialities, but
perhaps we can reduce it by few %...
I am traveling till end of week, but I will try to have something
ASAP...

Honza
> 
> 
> r~


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]