patch: remove PseudoSourceValue from Value.

[Eric Christopher]

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
>