[PATCH] D12920: Add TargetCustom type to PseudoSourceValue

Fri Sep 25 13:15:03 PDT 2015

----- Original Message -----
> From: "Marcello Maggioni" <mmaggioni at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D12920+public+8f10800ecfd8d899 at reviews.llvm.org, "Bbbbb" <llvm-commits at lists.llvm.org>, "Nick Lewycky"
> <nicholas at mxc.ca>
> Sent: Friday, September 25, 2015 3:11:10 PM
> Subject: Re: [PATCH] D12920: Add TargetCustom type to PseudoSourceValue
> 
> 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?
> 

I don't think this is happening any time soon, and regardless, we don't have an plan for doing so to which the community has agreed.

 -Hal

> 
> 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
> ).
> 
> -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
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory