Chris Lattner via llvm-dev
2020-Jul-24 20:06 UTC
[llvm-dev] [RFC] Requiring explicit address space arguments for PointerType
I agree, improving this makes sense. Alexander, I don’t think that “LLVM_DEFAULT_AS_PARAM” is the right way to go, I would just remove the “ = 0” default parameter entirely. -Chris> On Jul 23, 2020, at 11:02 AM, Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Hi Alex, > > I'd be very much in favor of this, thanks for pushing ;) > > On Wed, Jul 22, 2020 at 1:55 PM Alexander Richardson via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> (5?) Depending on how we feel about out-of-tree consumers, remove the >> LLVM_DEFAULT_AS_PARAM(AddressSpace) macros and require the argument to >> always be present. Adding the argument is compatible with older LLVM >> versions so it should be possible to do so without version #ifdefs. I >> think we should probably reduce the amount of changes required by >> out-of-tree consumers and keep the optional default, but it would >> obviously be nice to eventually remove the #ifdefs from inside LLVM. > > As you say, since consumers can be changed in a way where the new > consumer version still builds against older LLVM versions, I think > this is reasonable. It would be great to find a way where consumers > get deprecation warnings for some period of time, but I can't think of > an easy way to do that. > > Cheers, > Nicolai > > >> >> Does this sound like a sensible approach? Should I also attempt to >> implement steps 4 and 5 or is 1-3 sufficient? >> >> I would be great if we could land this upstream as it will >> significantly reduce the maintenance burden for our CHERI LLVM and >> should also prevent AVR issues such as e.g. >> https://reviews.llvm.org/rG215dc2e203341f7d1edc4c4a191b048af4ace43d >> >> Thanks, >> Alex >> >> [1] CHERI is currently available for MIPS and RISC-V and will soon be >> available on the upcoming Arm Morello board >> (https://developer.arm.com/architectures/cpu-architecture/a-profile/morello). >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > > > -- > Lerne, wie die Welt wirklich ist, > aber vergiss niemals, wie sie sein sollte. > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Johannes Doerfert via llvm-dev
2020-Jul-26 21:35 UTC
[llvm-dev] [RFC] Requiring explicit address space arguments for PointerType
On 7/24/20 3:06 PM, Chris Lattner via llvm-dev wrote:> I agree, improving this makes sense. > > Alexander, I don’t think that “LLVM_DEFAULT_AS_PARAM” is the right way to go, I would just remove the “ = 0” default parameter entirely.+1 on requiring everyone to provide the AS all the time. Something similar helped to remove alignment problems and confusion not too long ago.> -Chris > >> On Jul 23, 2020, at 11:02 AM, Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> Hi Alex, >> >> I'd be very much in favor of this, thanks for pushing ;) >> >> On Wed, Jul 22, 2020 at 1:55 PM Alexander Richardson via llvm-dev >> <llvm-dev at lists.llvm.org> wrote: >>> (5?) Depending on how we feel about out-of-tree consumers, remove the >>> LLVM_DEFAULT_AS_PARAM(AddressSpace) macros and require the argument to >>> always be present. Adding the argument is compatible with older LLVM >>> versions so it should be possible to do so without version #ifdefs. I >>> think we should probably reduce the amount of changes required by >>> out-of-tree consumers and keep the optional default, but it would >>> obviously be nice to eventually remove the #ifdefs from inside LLVM. >> As you say, since consumers can be changed in a way where the new >> consumer version still builds against older LLVM versions, I think >> this is reasonable. It would be great to find a way where consumers >> get deprecation warnings for some period of time, but I can't think of >> an easy way to do that. >> >> Cheers, >> Nicolai >> >> >>> Does this sound like a sensible approach? Should I also attempt to >>> implement steps 4 and 5 or is 1-3 sufficient? >>> >>> I would be great if we could land this upstream as it will >>> significantly reduce the maintenance burden for our CHERI LLVM and >>> should also prevent AVR issues such as e.g. >>> https://reviews.llvm.org/rG215dc2e203341f7d1edc4c4a191b048af4ace43d >>> >>> Thanks, >>> Alex >>> >>> [1] CHERI is currently available for MIPS and RISC-V and will soon be >>> available on the upcoming Arm Morello board >>> (https://developer.arm.com/architectures/cpu-architecture/a-profile/morello). >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> >> -- >> Lerne, wie die Welt wirklich ist, >> aber vergiss niemals, wie sie sein sollte. >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Alexander Richardson via llvm-dev
2020-Jul-27 09:55 UTC
[llvm-dev] [RFC] Requiring explicit address space arguments for PointerType
Hi Chris, While I'd love to simply remove the "= 0" parameter immediately, there are (after my initial patch to remove if from llvm/IR) currently at least 275 uses of getPointerTo() and 195 PointerType::getUnqual(). Unfortunately, it's not always obvious what the right address space should be. Should it be a pointer to code/globals/the stack? The reason I used the ugly LLVM_DEFAULT_AS_PARAM approach is so that I can incrementally remove those uses from certain directories and prevent new uses from being added. Preventing new uses is extremely important for our downstream fork, but not so much for most targets currently in upstream LLVM. Therefore, I will remove the =0 locally and submit incremental patches to remove those uses instead of using the LLVM_DEFAULT_AS_PARAM approach. Hopefully there won't be many new uses of the old APIs being added in the meantime. Once this is complete I'll add new deprecated overloads that still have the =0, to allow gradual migration for downstreams and remove them after a few weeks/months/when the next release branch is created. Does that sound like a better approach? Alex On Fri, 24 Jul 2020 at 21:06, Chris Lattner <clattner at nondot.org> wrote:> > I agree, improving this makes sense. > > Alexander, I don’t think that “LLVM_DEFAULT_AS_PARAM” is the right way to go, I would just remove the “ = 0” default parameter entirely. > > -Chris > > > On Jul 23, 2020, at 11:02 AM, Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > > > Hi Alex, > > > > I'd be very much in favor of this, thanks for pushing ;) > > > > On Wed, Jul 22, 2020 at 1:55 PM Alexander Richardson via llvm-dev > > <llvm-dev at lists.llvm.org> wrote: > >> (5?) Depending on how we feel about out-of-tree consumers, remove the > >> LLVM_DEFAULT_AS_PARAM(AddressSpace) macros and require the argument to > >> always be present. Adding the argument is compatible with older LLVM > >> versions so it should be possible to do so without version #ifdefs. I > >> think we should probably reduce the amount of changes required by > >> out-of-tree consumers and keep the optional default, but it would > >> obviously be nice to eventually remove the #ifdefs from inside LLVM. > > > > As you say, since consumers can be changed in a way where the new > > consumer version still builds against older LLVM versions, I think > > this is reasonable. It would be great to find a way where consumers > > get deprecation warnings for some period of time, but I can't think of > > an easy way to do that. > > > > Cheers, > > Nicolai > > > > > >> > >> Does this sound like a sensible approach? Should I also attempt to > >> implement steps 4 and 5 or is 1-3 sufficient? > >> > >> I would be great if we could land this upstream as it will > >> significantly reduce the maintenance burden for our CHERI LLVM and > >> should also prevent AVR issues such as e.g. > >> https://reviews.llvm.org/rG215dc2e203341f7d1edc4c4a191b048af4ace43d > >> > >> Thanks, > >> Alex > >> > >> [1] CHERI is currently available for MIPS and RISC-V and will soon be > >> available on the upcoming Arm Morello board > >> (https://developer.arm.com/architectures/cpu-architecture/a-profile/morello). > >> _______________________________________________ > >> LLVM Developers mailing list > >> llvm-dev at lists.llvm.org > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > > > > > > > -- > > Lerne, wie die Welt wirklich ist, > > aber vergiss niemals, wie sie sein sollte. > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
Chris Lattner via llvm-dev
2020-Jul-28 01:02 UTC
[llvm-dev] [RFC] Requiring explicit address space arguments for PointerType
On Jul 27, 2020, at 2:55 AM, Alexander Richardson <Alexander.Richardson at cl.cam.ac.uk> wrote:> > Hi Chris, > > While I'd love to simply remove the "= 0" parameter immediately, there > are (after my initial patch to remove if from llvm/IR) currently at > least 275 uses of getPointerTo() and 195 PointerType::getUnqual(). > Unfortunately, it's not always obvious what the right address space > should be. Should it be a pointer to code/globals/the stack? > > The reason I used the ugly LLVM_DEFAULT_AS_PARAM approach is so that I > can incrementally remove those uses from certain directories and > prevent new uses from being added. > Preventing new uses is extremely important for our downstream fork, > but not so much for most targets currently in upstream LLVM.Makes sense. The macro isn’t really appropriate for upstream, but I can see how it is super useful for your purposes.> > Therefore, I will remove the =0 locally and submit incremental patches > to remove those uses instead of using the LLVM_DEFAULT_AS_PARAM > approach.This is a great approach.> Hopefully there won't be many new uses of the old APIs being added in > the meantime. > Once this is complete I'll add new deprecated overloads that still > have the =0, to allow gradual migration for downstreams and remove > them after a few weeks/months/when the next release branch is created. > Does that sound like a better approach?Yes, this sounds awesome. Thank you Alexander! -Chris