[PATCH] D12920: Add TargetCustom type to PseudoSourceValue
Marcello Maggioni via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 25 13:11:10 PDT 2015
Yeah, that was my understanding and it was I meant to do.
The fact is that it was mentioned that we shouldn’t use PseudoSourceValue, because it was planned to be removed .
But probably that is not the case apparently?
Thanks for the comment on the patch. I’ll take a look at the link you posted and do what is needed :)
Marcello
> On 22 Sep 2015, at 16:44, Hal Finkel <hfinkel at anl.gov> wrote:
>
> ----- Original Message -----
>> From: "Marcello Maggioni via llvm-commits" <llvm-commits at lists.llvm.org>
>> To: "Nick Lewycky" <nicholas at mxc.ca>
>> Cc: reviews+D12920+public+8f10800ecfd8d899 at reviews.llvm.org, "LLVM Commits" <llvm-commits at lists.llvm.org>
>> Sent: Friday, September 18, 2015 1:17:15 AM
>> Subject: Re: [PATCH] D12920: Add TargetCustom type to PseudoSourceValue
>>
>> HI Nick, thanks for the comment.
>>
>> When you mention "use MachinePointerInfo" what do you mean exactly?
>>
>> Creating one leaving the Value to null.
>> The approach I was using was creating a MachinePointerInfo and
>> attaching target specific data to a PSV and pass the PSV to the
>> constructor of the MachinePointerInfo. The idea was to use that
>> target specific data later on in target passes or in functions like
>> "isTriviallyDisjoint".
>> MachinePointerInfo alone doesn't seem to have any way of storing such
>> data with the exclusion of the offset field. Are you suggesting of
>> using that to deliver target specific information?
>
> I don't think that sticking non-offset data into the Offset field is a good idea. Target-independent code might legitimately interpret that field as specifying a relative offset even if it does not understand the underlying PseudoSourceValue.
>
> I would expect that you would derive a subclass from PseudoSourceValue and give it whatever fields were necessary to store your target-specific data, just as we currently do with the target-independent subclasses of PseudoSourceValue (FixedStackPseudoSourceValue, etc.)
>
> Also, please re-post your patch with full context (see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface <http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface>).
>
> -Hal
>
>>
>> Thanks,
>> Marcello
>>
>> Sent from my iPhone
>>
>>> On Sep 17, 2015, at 11:00 PM, Nick Lewycky via llvm-commits
>>> <llvm-commits at lists.llvm.org> wrote:
>>>
>>> Pete Cooper wrote:
>>>> I might be mis-remembering, but I thought we were trying to remove
>>>> PSV’s?
>>>>
>>>> Only commit I could see which remotely suggested that might be the
>>>> case is r206255 where the comment contained
>>>>
>>>> "Anything that needs to use either a PseudoSourceValue* and Value*
>>>> is strongly encouraged to use a MachinePointerInfo instead.”
>>>>
>>>> I’ve CCed Nick to see if he can remember whether there was more
>>>> work to come on PSV’s or whether they are here to stay. If they
>>>> are going to be removed in future then its something to think
>>>> about here before adding more code which relies on them.
>>>
>>> The really important part happened, PSV's no longer derive from
>>> Value*. The advice about using MachinePointerInfo when possible
>>> remains true, but my work on removing PSV entirely is unlikely to
>>> be completed.
>>>
>>> Nick
>>>
>>>>
>>>> Pete
>>>>> On Sep 17, 2015, at 5:51 PM, Marcello Maggioni via
>>>>> llvm-commits<llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>> kariddi updated this revision to Diff 35051.
>>>>> kariddi marked an inline comment as done.
>>>>> kariddi added a comment.
>>>>>
>>>>> Still waiting for comments about the MIR serialization stuff.
>>>>>
>>>>>
>>>>> http://reviews.llvm.org/D12920
>>>>>
>>>>> Files:
>>>>> include/llvm/CodeGen/PseudoSourceValue.h
>>>>> lib/CodeGen/MIRPrinter.cpp
>>>>>
>>>>> Index: lib/CodeGen/MIRPrinter.cpp
>>>>> ===================================================================
>>>>> --- lib/CodeGen/MIRPrinter.cpp
>>>>> +++ lib/CodeGen/MIRPrinter.cpp
>>>>> @@ -892,6 +892,9 @@
>>>>> printLLVMNameWithoutPrefix(
>>>>> OS,
>>>>> cast<ExternalSymbolPseudoSourceValue>(PVal)->getSymbol());
>>>>> break;
>>>>> + case PseudoSourceValue::TargetCustom:
>>>>> + llvm_unreachable("TargetCustom pseudo source values are
>>>>> not supported");
>>>>> + break;
>>>>> }
>>>>> }
>>>>> printOffset(Op.getOffset());
>>>>> Index: include/llvm/CodeGen/PseudoSourceValue.h
>>>>> ===================================================================
>>>>> --- include/llvm/CodeGen/PseudoSourceValue.h
>>>>> +++ include/llvm/CodeGen/PseudoSourceValue.h
>>>>> @@ -40,7 +40,8 @@
>>>>> ConstantPool,
>>>>> FixedStack,
>>>>> GlobalValueCallEntry,
>>>>> - ExternalSymbolCallEntry
>>>>> + ExternalSymbolCallEntry,
>>>>> + TargetCustom
>>>>> };
>>>>>
>>>>> private:
>>>>> @@ -63,6 +64,9 @@
>>>>> bool isGOT() const { return Kind == GOT; }
>>>>> bool isConstantPool() const { return Kind == ConstantPool; }
>>>>> bool isJumpTable() const { return Kind == JumpTable; }
>>>>> + unsigned getTargetCustom() const {
>>>>> + return (Kind>= TargetCustom) ? ((Kind+1) - TargetCustom) :
>>>>> 0;
>>>>> + }
>>>>>
>>>>> /// Test whether the memory pointed to by this
>>>>> PseudoSourceValue has a
>>>>> /// constant value.
>>>>>
>>>>>
>>>>> <D12920.35051.patch>_______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150925/be614363/attachment.html>
More information about the llvm-commits
mailing list