patch: remove PseudoSourceValue from Value.

[Nick Lewycky]

On 10 March 2014 22:01, Chris Lattner <clattner at apple.com> wrote:

>
> On Mar 10, 2014, at 5:56 PM, Nick Lewycky <nlewycky at google.com> wrote:
>
>  My personal problem with PseudoSourceValue is that is perverts the Value
>> hierarchy with things that are not IR values... originally so that the AA
>> interfaces could take machine "pointers" and somehow do something sensible
>> with them.
>>
>> This is a flawed design.  The only way this could work is if the
>> "MachineAA" level interface successfully catches any (e.g.) stack address
>> entries and avoids passing them do into the IR-level AA interfaces.
>>
>> Given that, we're getting no benefit from mashing them in as Value*'s and
>> we're also getting no help from the C++ type system.  A better way to go
>> would be to make PseudoSourceValue be a pointer-sized discriminated union
>> that is *either* a Value*, a frame index, constant pool index, etc.  This
>> would require clients to unwrap all the special cases (verifying they
>> actually have a value*) before calling the IR level interfaces, and it
>> would give us a place to put various query APIs (on PseudoSourceValue
>> itself).
>>
>> While we're at it, it should be renamed to something that starts with
>> "Machine". :-)
>>
>
> This sounds very similar to what I've done in the patch I mailed out.
> PseudoSourceValue is changed to represent only frame index, constant pool
> index, etc., never a Value*. MachinePointerInfo stores a pointer union of
> Value* and PseudoSourceValue.
>
> Then you'd like me to merge PSV into MPI? There are methods on PSV which
> aren't defined on MPI, so I'm not sure what I would do about them -- make
> them assert the MPI isn't holding a Value*?
>
>
> Which methods?  I haven't looked at the patch, but it sounds like
> MachinePointerInfo is the right thing.  Can you just nuke PSV from orbit?
> :-)
>

Very difficult.

The main problem is that PSVs are uniqued upon creation.
lib/CodeGen/PseudoSourceValue.cpp maintains a std::map in a ManagedStatic
global variable which ensures that there is only one address for the PSVs.
This is outside any llvmcontext, so that code ultimately needs to be
deleted, but there are consumers relying on the uniquing it provides,
namely the ScheduleDAGInstrs AliasMem maps and the Mips delay slot filler.

There's also some interesting differences because PSVs are passed by
pointer while MachinePointerInfo is passed by value. It means more changes
to more APIs.

The next problem is MipsCallEntry which *derives* from PseudoSourceValue,
but never declared it in the Value enum list. There is no way that possibly
works, and I'm not yet sure what to do about it.

Chris, breaking PSV off of Value is a reasonable first step. Can we start
there? Or do you *really* want a larger patch that removes PSV all in one
go?

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140414/c541b118/attachment.html>