[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