patch: remove PseudoSourceValue from Value.

[Nick Lewycky]

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

>
> On Mar 4, 2014, at 9:12 PM, Nick Lewycky <nlewycky at google.com> wrote:
>
> On 4 March 2014 13:59, Evan Cheng <evan.cheng at apple.com> wrote:
>
>> Sorry, I need to have better mail filtering.
>>
>> PseudoSourceValue is a bad hack that's long on my kill list. Thank you
>> for tackling this. Would you consider going further than your current patch
>> and kill it completely?
>>
>
> I'm trying to get some undefined behaviour removed from LLVM, and I'm
> pulling the thread of llvm::Value having a vtable. This would be a
> diversion ...
>
> ... but I realize that doing this patch now and then removing PSV later
> would break out of tree targets twice, and it would be nice to avoid that.
> That's a good reason to do it all at once.
>
> The idea is to enhance MachinePointerInfo so the value can be an IR
>> pointer value (i.e. const Value *V) or some immediate that represents
>> special type such as constant pool, stack objects, etc. As a bonus, we
>> would also want a mechanism to represent target specific tag to model
>> special dependencies (common in non-CPU world). Your change in
>> MachineMemOperand.h seems half way there.
>>
>
> Could you expand on this? How would that immediate be different from
> PseudoSourceValue after this change? Would we have the four PSVs (stack,
> GOT, JumpTable, ConstantPool) plus a Value PSV that contains a Value*? And
> targets could add their own to the list of 4 I mentioned?
>
> Basically, I don't know enough to fill in the details of design you're
> describing, but I'm willing to do the actual coding/testing/debugging bits.
>
>
> 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*?

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