llvm dev - Feb 2010 - [LLVMdev] Test approach to handling clobbering llvm.eh.selector return

Hi Duncan,

I hacked together a version of DwarfEHPrepare.cpp which tries to deal with the
ordering of llvm.eh.exception and llvm.eh.selector.
The hacked is contained within the attached patch. 

Motivation:

I recently created a decent amount of hand coded IR (via the llvm C++ API). In
order to help me runtime debug the code,
I created automatic constructors which would trace entries into the blocks
depending on the setting of a build flag. This means
that the first instruction to every block was a call to fprintf. The trace can
be delayed for scenarios such as blocks with phi nodes.
For landing pads however, I ran into the issue where the runtime call to fprint
seemed to affect the same register used as that used
by llvm.eh.selector's return. When tracing was turned on then, I got garbage
for llvm.eh.selector's return. As you stated in a
previous email this should be fixed in a similar manner as that given for
llvm.eh.exception in DwarfEHPrepare. You also stated
there were issues, which I believe I partially understand (see bottom).

Its goals are:

1) To move llvm.eh.selector calls immediately after llvm.eh.exception calls
within landing pads
2) To force the exception argument of llvm.eh.selector to come from the return
of the preceding llvm.eh.exception call.
3) To create and move loads of llvm.eh.selector immediately after newly created
loads of llvm.eh.exception in non-landing pads.
4) To add the relevant stores (after steps 1 and 2 are finished for both
llvm.eh.exception and llvm.eh.selector) for llvm.eh.selector
5) To include loads and stores of llvm.eh.selector to the register promotion
optimization.
6) To include statistics for llvm.eh.selector call "moves".

The following paths were manually tested:

1) The movement of llvm.eh.exception calls (pre-existing functionality), and
llvm.eh.selector within landing pads.
2) The above path with the additional change of llvm.eh.selector's exception
argument as described in goal 2.

The following paths were NOT tested:

1) Non-landing transformations of llvm.eh.selector calls to Loads, along with
the required inclusion of stores
2) All the complicated scenarios that these algorithms may have to deal with.

Two implementations, who's choice is determined by hard coded preprocessor
if, are given:

1) Following the pre-existing code patterns for the movement of landing pad
llvm.eh.exception calls,
	llvm.eh.selector calls (when necessary), are cloned and inserted at the correct
position with
	the old llvm.eh.selector call "removed". In this implementation a
necessitated changed of the new
	llvm.eh.selector's exception argument results in the old instance of this
argument not being removed
	even if its use drops to zero. Because the old llvm.eh.selector call is not
replace by the new
	llvm.eh.selector call until after the exception argument is changed (if
necessary), the old exception
	argument will have at least 1 use at the point where the "new"
exception argument is applied to the
	new llvm.eh.selector call. The code, at this point, could remove the exception
argument from the old
	llvm.eh.selector call, but this treatment would not follow the replace
semantics of pre-existing code.
2) The llvm.eh.selector is instead moved to the correct position (using
llvm.Instruction.moveBefore(...)), with
	any necessary exception argument change. This version required a change in the
control flow and is
	therefore larger addition to DwarfEHPrepare.

Problems:

Although the patch deals with the llvm.eh.selector's exception argument, it
does not know how to deal with
its other arguments. This would not be a problem for say type infos, and I
believe the personality function since
these arguments must be direct references to Globals. However I'm unclear on
filters, as I have never used
them. Regardless I ignored this issue since you have a full understanding as to
whether this is an issue or
not, and as to how to fix it if it is relevant. 

Some of the relevant changes are marked with commented a GMV:, which would be
removed for the real version.
In addition I have not yet added comments, preferring to delay until and if you
think this fix is worth anything.

Hopefully this effort is not too obfuscated for your use.

Garrison

-------------- next part --------------
A non-text attachment was scrubbed...
Name: DwarfEHPrepare.patch
Type: application/octet-stream
Size: 15887 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100208/1fb28e61/attachment.obj>
-------------- next part --------------

Hi Garrison,
> I hacked together a version of DwarfEHPrepare.cpp which tries to deal with
the ordering of llvm.eh.exception and llvm.eh.selector.
> The hacked is contained within the attached patch.
it looks like you tried to copy the code for eh.exception.  There are two
problems with this: (1) the eh.exception code really needs to be rewritten
to make use of the new SSAUpdator (then all the mucking around with domtree
can go away; it's quite tricky to preserve dominfo when updating the IR, and
the whole dom approach is inefficient), and (2) the eh.selector case is much
more complicated because selectors carry type infos (and filters) around with
them.  Sadly, you can just move selectors to the landing pad(s) their exception
objects come from, since you might end up with multiple selectors there, and
because the order of type infos matters, you can't just concatenate the type
info lists.  It's worse for filters because these have extra, hidden,
semantics
that are enforced by the personality function.
> 2) To force the exception argument of llvm.eh.selector to come from the
return of the preceding llvm.eh.exception call.
This sounds bogus.  llvm.eh.selector compares it's exception argument with
the
list of type infos, and returns an appropriate index.  There is no connection
with "the current exception" in general, or the preceeding
llvm.eh.exception
call in particular.  This is one of the problems dealing with eh.selector when
the call is far from the landing pad where the exception was caught.  That said,
this doesn't work properly right now anyway.
> 5) To include loads and stores of llvm.eh.selector to the register
promotion optimization.
The register promotion should be reworked using SSAUpdator, as I mentioned
above.
> Although the patch deals with the llvm.eh.selector's exception
argument, it does not know how to deal with
> its other arguments. This would not be a problem for say type infos, and I
believe the personality function since
> these arguments must be direct references to Globals. However I'm
unclear on filters, as I have never used
> them. Regardless I ignored this issue since you have a full understanding
as to whether this is an issue or
> not, and as to how to fix it if it is relevant. 
See above.

Ciao,

Duncan.

Bummer, I had a feeling I was only solving the test case that pertained to my
problem: the clobbering of the
register, used by the selector's return, by intervening instructions (those
between the llvm.eh.exception call
and the llvm.eh.selector call). The forcing of the selector's exception
argument got rid of a bitcast problem,
and I imagined any other potential operation, that manifested itself after the
reorder. So now I've got a hacked
version of DwarfEHPrepare that only works for my use case. :-) I guess it is
time to look at SSAUpdator even
though this will not fix the selector argument problem.

So my new conservative IR generation resolution is:

1) Execute llvm.eh.exception as the first instruction of the landing pad.
2) Execute llvm.eh.selector as the second instruction of that landing pad.

Although llvm currently allows more flexibility, holding to those two
resolutions will potentially avoid the trashing of llvm.eh.selector's 
return.

Thanks for the response

Garrison



On Feb 11, 2010, at 15:34, Duncan Sands wrote:
> Hi Garrison,
> 
>> I hacked together a version of DwarfEHPrepare.cpp which tries to deal
with the ordering of llvm.eh.exception and llvm.eh.selector.
>> The hacked is contained within the attached patch.
> 
> it looks like you tried to copy the code for eh.exception.  There are two
> problems with this: (1) the eh.exception code really needs to be rewritten
> to make use of the new SSAUpdator (then all the mucking around with domtree
> can go away; it's quite tricky to preserve dominfo when updating the
IR, and
> the whole dom approach is inefficient), and (2) the eh.selector case is
much
> more complicated because selectors carry type infos (and filters) around
with
> them.  Sadly, you can just move selectors to the landing pad(s) their
exception
> objects come from, since you might end up with multiple selectors there,
and
> because the order of type infos matters, you can't just concatenate the
type
> info lists.  It's worse for filters because these have extra, hidden,
semantics
> that are enforced by the personality function.
> 
>> 2) To force the exception argument of llvm.eh.selector to come from the
return of the preceding llvm.eh.exception call.
> 
> This sounds bogus.  llvm.eh.selector compares it's exception argument
with the
> list of type infos, and returns an appropriate index.  There is no
connection
> with "the current exception" in general, or the preceeding
llvm.eh.exception
> call in particular.  This is one of the problems dealing with eh.selector
when
> the call is far from the landing pad where the exception was caught.  That
said,
> this doesn't work properly right now anyway.
> 
>> 5) To include loads and stores of llvm.eh.selector to the register
promotion optimization.
> 
> The register promotion should be reworked using SSAUpdator, as I mentioned
> above.
> 
>> Although the patch deals with the llvm.eh.selector's exception
argument, it does not know how to deal with
>> its other arguments. This would not be a problem for say type infos,
and I believe the personality function since
>> these arguments must be direct references to Globals. However I'm
unclear on filters, as I have never used
>> them. Regardless I ignored this issue since you have a full
understanding as to whether this is an issue or not, and as to how to fix it if
it is relevant.
> 
> See above.
> 
> Ciao,
> 
> Duncan.
>