patch: remove PseudoSourceValue from Value.

[Nick Lewycky]

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.

Nick


>
> Evan
>
> On Mar 4, 2014, at 1:37 PM, Eric Christopher <echristo at gmail.com> wrote:
>
>  On Mon, Mar 3, 2014 at 6:09 PM, Nick Lewycky <nlewycky at google.com> wrote:
>
> 1 week ping!
>
> +Eric Christopher, I'd also appreciate a review.
>
>
> Sure. It's a pretty large patch, I noticed a few things and really
> hope we have testcases for a lot of it :)
>
> +    bool isFixed;
>
> Appears to be completely undocumented :)
>
>
> +  template<typename T, typename U>
> +  struct DenseMapInfo<PointerUnion<T, U> > {
>
> As does this :)
>
> +                                  DstInfo, MachinePointerInfo((Value*)0));
>
> MachinePointerInfo() ?
>
> (and in the other places that have the same idiom)
>
> // Eventually these should be uniqued on LLVMContext rather than in a
> managed
> // static.  For now, we can safely use the global context for the time
> being to
> // squeak by.
> -PseudoSourceValue::PseudoSourceValue(enum ValueTy Subclass) :
> -  Value(Type::getInt8PtrTy(getGlobalContext()),
> -        Subclass) {}
> +PseudoSourceValue::PseudoSourceValue(bool isFixed) : isFixed(isFixed) {}
>
> Comment appears out of date now.
>
> Otherwise looks ok to me.
>
> -eric
>
>
> Hal, do you have any further review comments? (Does it look good to you,
> but
> you don't feel like you have sufficient knowledge to OK a commit?)
>
> I think that between the three of us, Hal, Eric and myself, that would be
> sufficient eyes to approve the patch for commit.
>
> As a note to everybody else, this is likely to break out of tree targets
> when it lands. My current plan is to send an email to llvm-dev shortly
> before committing this patch, after I get the OK to commit.
>
> Nick
>
>
> On 24 February 2014 17:19, Nick Lewycky <nlewycky at google.com> wrote:
>
>
> +cc Evan Cheng
>
> Evan, I'm not sure whether you're still the right person to review this. I
> consider it a substantive change to how the backend works, even though the
> line count of the patch is small. If nothing else, I figure you want to be
> aware of this patch before it lands.
>
> Nick
>
>
> On 24 February 2014 17:01, Nick Lewycky <nlewycky at google.com> wrote:
>
>
> On 24 February 2014 16:45, Hal Finkel <hfinkel at anl.gov> wrote:
>
>
> ----- Original Message -----
>
> From: "Nick Lewycky" <nlewycky at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, February 24, 2014 6:31:34 PM
> Subject: Re: patch: remove PseudoSourceValue from Value.
>
>
>
>
> On 24 February 2014 16:20, Hal Finkel < hfinkel at anl.gov > wrote:
>
>
> What is the underlying motivation for doing this?
>
>
>
> The proximate cause is that I'm trying to remove the vtable from
> llvm::Value, and in order to do that I'm going to write dispatch
> functions that switch on the ValueID and call the appropriate
> function from the most-derived-type. If I do that, then we have
> lib/IR placing calls to lib/CodeGen, which is a layering violation.
>
>
> At a higher level, PSV does not belong there. It is not part of the
> IR. No IR passes use or understand it. It's not documented in the
> LangRef. Frankly, it's a hack -- and it's a great hack that and I
> understand its utility for lib/CodeGen -- but that doesn't change
> the fact that it's not part of llvm's IR. I've just never been
> motivated to do something about it until now.
>
>
> Thanks for explaining!
>
>   AliasAnalysis::AliasResult AAResult = AA->alias(
> -  AliasAnalysis::Location(MMOa->getValue(), Overlapa,
> -                          UseTBAA ? MMOa->getTBAAInfo() : 0),
> -  AliasAnalysis::Location(MMOb->getValue(), Overlapb,
> -                          UseTBAA ? MMOb->getTBAAInfo() : 0));
> +      AliasAnalysis::Location(MMOa->getValue(), Overlapa,
> +                              UseTBAA ? MMOa->getTBAAInfo() : 0),
> +      AliasAnalysis::Location(MMOb->getValue(), Overlapb,
> +                              UseTBAA ? MMOb->getTBAAInfo() : 0));
>
> This is purely an indentation change, can you please commit this
> separately?
>
>
>
> Done.
>
>
> -        MapVector<const Value *, std::vector<SUnit *> >::iterator IE =
> +        MapVector<PointerUnion<const Value *, const PseudoSourceValue
> *>,
> +            std::vector<SUnit *> >::iterator IE =
>
> These are starting to get really ugly. Can you please do something so
> that we don't have to use these really long local types in many places
> (like
> make a local typedef)?
>
>
>
> Done. Updated patch attached.
>
> Nick
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140304/94827902/attachment.html>