patch: remove PseudoSourceValue from Value.

[Evan Cheng]

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?

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.

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/b2f5d18c/attachment.html>