[PATCH] D12920: Add TargetCustom type to PseudoSourceValue

[Hal Finkel via llvm-commits]

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