Kuperstein, Michael M wrote:> Thanks Nick! > > Can you just check sanity before I commit? > (Or suggest a better name for the function...)Sure! +static bool canNotAliasDiffArgument(const Value *V) +{ + return (isa<AllocaInst>(V) || isNoAliasCall(V) || isNoAliasArgument(V)); +} Extra parens? The name "canNotAliasDiffArgument" works, but it's named for what we need the function to do in its sole caller. Thinking about what it does without a specific caller in mind, I might think of "noaliasAtFunctionEntry", as it indicates that the object was noalias if you assume no instructions in the function have had the chance to run yet (to make aliasing copies). +/// isNoAliasArgument - Return true if this is an argument with the noalias +/// attribute. +bool isNoAliasArgument(const Value *V); Hold on: any reason this needs to be in the llvm namespace? It has a single caller in BasicAA, maybe move it there? Nick> > -----Original Message----- > From: Nick Lewycky [mailto:nicholas at mxc.ca] > Sent: Sunday, May 26, 2013 09:54 > To: Kuperstein, Michael M > Cc: LLVMdev at cs.uiuc.edu > Subject: Re: [LLVMdev] Mixing noalias and regular arguments > > Kuperstein, Michael M wrote: >> Ping? > > Pong! Sorry for the slow review, I had this patch starred but hadn't got around to it. Yes, the rationale and implementation are correct. > >> (Is there a code owner for AA, btw?) > > (It falls back on the more general code owner who is Chris Lattner in this case, "Everything not covered by someone else".) > > +/// isNoAliasArgument - Return true if this is an argument with the > +noalias /// attribute. > +bool isNoAliasArgument(const Value* V); > > "const Value* V" should be "const Value *V". > > + // Arguments can't alias with noalias arguments > + if ((isa<Argument>(O1)&& isNoAliasArgument(O2)) || > + (isa<Argument>(O2)&& isNoAliasArgument(O1))) > + return NoAlias; > > Fold this into the logic right above it: > > // Arguments can't alias with local allocations or noalias calls > // in the same function. > if (((isa<Argument>(O1)&& (isa<AllocaInst>(O2) || > isNoAliasCall(O2))) || > (isa<Argument>(O2)&& (isa<AllocaInst>(O1) || > isNoAliasCall(O1))))) > return NoAlias; > > by factoring out the combined tests "isa<AllocaInst>(V) || > isNoAliasCall(V) || isNoAliasArgument(V)" into a function. > > +/// isNoAliasArgument - Return true if this is an argument with the > +noalias /// attribute. > +bool llvm::isNoAliasArgument(const Value* V) { > + if (const Argument *A = dyn_cast<Argument>(V)) > > Please be consistent, "const Value* V" vs. "const Argument *A". This file puts the star on the right of the space. > > Thanks for fixing this. Please commit once you've addressed the above! > > Nick > >> *From:*llvmdev-bounces at cs.uiuc.edu >> [mailto:llvmdev-bounces at cs.uiuc.edu] >> *On Behalf Of *Kuperstein, Michael M >> *Sent:* Tuesday, May 21, 2013 18:23 >> *To:* LLVMdev at cs.uiuc.edu >> *Cc:* Raoux, Thomas F >> *Subject:* [LLVMdev] Mixing noalias and regular arguments >> >> Hi all, >> >> I'm trying to understand the semantics of noalias arguments, and I'm >> not entirely sure I got it correctly. >> >> To the best of my understanding, if an argument is declared noalias, >> "This indicates that pointer values based on the argument do not alias >> pointer values which are not based on it" implies, among other things, >> that it cannot alias any other argument, even if that argument is NOT >> declared noalias. >> >> However, currently, BasicAliasAnalysis doesn't recognize this case >> explicitly. Sometimes it will work for other reasons (e.g. if it knows >> the other argument does not get captured), but it's relatively easy to >> get circumstances where the result is MayAlias. >> >> I'm attaching a patch that addresses this. >> >> Can anyone offer an opinion on the basic issue and, assuming this is >> the desired behavior, on the patch? >> >> Thanks, >> >> Michael > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies.
Kuperstein, Michael M
2013-May-27 11:37 UTC
[LLVMdev] Mixing noalias and regular arguments
Regarding the name, I'm not sure noaliasAtFunctionEntry makes sense, for two reasons: a) The pointer involved may not be defined at entry. b) We already have something which is similar in spirit - IsIdentifiedObject. That covers the 3 cases I have in the new function, but also two additional ones: GlobalValues which are not GlobalAliases, and byval arguments. Unfortunately, a check of Argument vs. IsIdentifiedObject is too broad, because an argument (which is not noalias) can alias a GlobalValue. Regarding the namespace - I just thought I should keep isNoAliasArgument close to isNoAliasCall. I'll move it to BasicAliasAnalysis if you think that's a better idea. -----Original Message----- From: Nick Lewycky [mailto:nicholas at mxc.ca] Sent: Monday, May 27, 2013 13:54 To: Kuperstein, Michael M Cc: LLVMdev at cs.uiuc.edu Subject: Re: [LLVMdev] Mixing noalias and regular arguments Kuperstein, Michael M wrote:> Thanks Nick! > > Can you just check sanity before I commit? > (Or suggest a better name for the function...)Sure! +static bool canNotAliasDiffArgument(const Value *V) { + return (isa<AllocaInst>(V) || isNoAliasCall(V) || +isNoAliasArgument(V)); } Extra parens? The name "canNotAliasDiffArgument" works, but it's named for what we need the function to do in its sole caller. Thinking about what it does without a specific caller in mind, I might think of "noaliasAtFunctionEntry", as it indicates that the object was noalias if you assume no instructions in the function have had the chance to run yet (to make aliasing copies). +/// isNoAliasArgument - Return true if this is an argument with the +noalias /// attribute. +bool isNoAliasArgument(const Value *V); Hold on: any reason this needs to be in the llvm namespace? It has a single caller in BasicAA, maybe move it there? Nick> > -----Original Message----- > From: Nick Lewycky [mailto:nicholas at mxc.ca] > Sent: Sunday, May 26, 2013 09:54 > To: Kuperstein, Michael M > Cc: LLVMdev at cs.uiuc.edu > Subject: Re: [LLVMdev] Mixing noalias and regular arguments > > Kuperstein, Michael M wrote: >> Ping? > > Pong! Sorry for the slow review, I had this patch starred but hadn't got around to it. Yes, the rationale and implementation are correct. > >> (Is there a code owner for AA, btw?) > > (It falls back on the more general code owner who is Chris Lattner in > this case, "Everything not covered by someone else".) > > +/// isNoAliasArgument - Return true if this is an argument with the > +noalias /// attribute. > +bool isNoAliasArgument(const Value* V); > > "const Value* V" should be "const Value *V". > > + // Arguments can't alias with noalias arguments > + if ((isa<Argument>(O1)&& isNoAliasArgument(O2)) || > + (isa<Argument>(O2)&& isNoAliasArgument(O1))) > + return NoAlias; > > Fold this into the logic right above it: > > // Arguments can't alias with local allocations or noalias calls > // in the same function. > if (((isa<Argument>(O1)&& (isa<AllocaInst>(O2) || > isNoAliasCall(O2))) || > (isa<Argument>(O2)&& (isa<AllocaInst>(O1) || > isNoAliasCall(O1))))) > return NoAlias; > > by factoring out the combined tests "isa<AllocaInst>(V) || > isNoAliasCall(V) || isNoAliasArgument(V)" into a function. > > +/// isNoAliasArgument - Return true if this is an argument with the > +noalias /// attribute. > +bool llvm::isNoAliasArgument(const Value* V) { > + if (const Argument *A = dyn_cast<Argument>(V)) > > Please be consistent, "const Value* V" vs. "const Argument *A". This file puts the star on the right of the space. > > Thanks for fixing this. Please commit once you've addressed the above! > > Nick > >> *From:*llvmdev-bounces at cs.uiuc.edu >> [mailto:llvmdev-bounces at cs.uiuc.edu] >> *On Behalf Of *Kuperstein, Michael M >> *Sent:* Tuesday, May 21, 2013 18:23 >> *To:* LLVMdev at cs.uiuc.edu >> *Cc:* Raoux, Thomas F >> *Subject:* [LLVMdev] Mixing noalias and regular arguments >> >> Hi all, >> >> I'm trying to understand the semantics of noalias arguments, and I'm >> not entirely sure I got it correctly. >> >> To the best of my understanding, if an argument is declared noalias, >> "This indicates that pointer values based on the argument do not >> alias pointer values which are not based on it" implies, among other >> things, that it cannot alias any other argument, even if that >> argument is NOT declared noalias. >> >> However, currently, BasicAliasAnalysis doesn't recognize this case >> explicitly. Sometimes it will work for other reasons (e.g. if it >> knows the other argument does not get captured), but it's relatively >> easy to get circumstances where the result is MayAlias. >> >> I'm attaching a patch that addresses this. >> >> Can anyone offer an opinion on the basic issue and, assuming this is >> the desired behavior, on the patch? >> >> Thanks, >> >> Michael > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies.--------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Kuperstein, Michael M wrote:> Regarding the name, I'm not sure noaliasAtFunctionEntry makes sense, for two reasons: > a) The pointer involved may not be defined at entry.Right, I thought of that too.> b) We already have something which is similar in spirit - IsIdentifiedObject. That covers the 3 cases I have in the new function, but also two additional ones: GlobalValues which are not GlobalAliases, and byval arguments.Okay.> Unfortunately, a check of Argument vs. IsIdentifiedObject is too broad, because an argument (which is not noalias) can alias a GlobalValue.Right. IsIdentifiedFunctionLocal? :)> Regarding the namespace - I just thought I should keep isNoAliasArgument close to isNoAliasCall. > I'll move it to BasicAliasAnalysis if you think that's a better idea.I generally avoid adding things to llvm:: when I can avoid it, but I don't care a whole lot. If you think it's a utility that could be used more generally, llvm:: is fine. Nick> > -----Original Message----- > From: Nick Lewycky [mailto:nicholas at mxc.ca] > Sent: Monday, May 27, 2013 13:54 > To: Kuperstein, Michael M > Cc: LLVMdev at cs.uiuc.edu > Subject: Re: [LLVMdev] Mixing noalias and regular arguments > > Kuperstein, Michael M wrote: >> Thanks Nick! >> >> Can you just check sanity before I commit? >> (Or suggest a better name for the function...) > > Sure! > > +static bool canNotAliasDiffArgument(const Value *V) { > + return (isa<AllocaInst>(V) || isNoAliasCall(V) || > +isNoAliasArgument(V)); } > > Extra parens? > > The name "canNotAliasDiffArgument" works, but it's named for what we need the function to do in its sole caller. Thinking about what it does without a specific caller in mind, I might think of "noaliasAtFunctionEntry", as it indicates that the object was noalias if you assume no instructions in the function have had the chance to run yet (to make aliasing copies). > > +/// isNoAliasArgument - Return true if this is an argument with the > +noalias /// attribute. > +bool isNoAliasArgument(const Value *V); > > Hold on: any reason this needs to be in the llvm namespace? It has a single caller in BasicAA, maybe move it there? > > Nick > >> >> -----Original Message----- >> From: Nick Lewycky [mailto:nicholas at mxc.ca] >> Sent: Sunday, May 26, 2013 09:54 >> To: Kuperstein, Michael M >> Cc: LLVMdev at cs.uiuc.edu >> Subject: Re: [LLVMdev] Mixing noalias and regular arguments >> >> Kuperstein, Michael M wrote: >>> Ping? >> >> Pong! Sorry for the slow review, I had this patch starred but hadn't got around to it. Yes, the rationale and implementation are correct. >> >>> (Is there a code owner for AA, btw?) >> >> (It falls back on the more general code owner who is Chris Lattner in >> this case, "Everything not covered by someone else".) >> >> +/// isNoAliasArgument - Return true if this is an argument with the >> +noalias /// attribute. >> +bool isNoAliasArgument(const Value* V); >> >> "const Value* V" should be "const Value *V". >> >> + // Arguments can't alias with noalias arguments >> + if ((isa<Argument>(O1)&& isNoAliasArgument(O2)) || >> + (isa<Argument>(O2)&& isNoAliasArgument(O1))) >> + return NoAlias; >> >> Fold this into the logic right above it: >> >> // Arguments can't alias with local allocations or noalias calls >> // in the same function. >> if (((isa<Argument>(O1)&& (isa<AllocaInst>(O2) || >> isNoAliasCall(O2))) || >> (isa<Argument>(O2)&& (isa<AllocaInst>(O1) || >> isNoAliasCall(O1))))) >> return NoAlias; >> >> by factoring out the combined tests "isa<AllocaInst>(V) || >> isNoAliasCall(V) || isNoAliasArgument(V)" into a function. >> >> +/// isNoAliasArgument - Return true if this is an argument with the >> +noalias /// attribute. >> +bool llvm::isNoAliasArgument(const Value* V) { >> + if (const Argument *A = dyn_cast<Argument>(V)) >> >> Please be consistent, "const Value* V" vs. "const Argument *A". This file puts the star on the right of the space. >> >> Thanks for fixing this. Please commit once you've addressed the above! >> >> Nick >> >>> *From:*llvmdev-bounces at cs.uiuc.edu >>> [mailto:llvmdev-bounces at cs.uiuc.edu] >>> *On Behalf Of *Kuperstein, Michael M >>> *Sent:* Tuesday, May 21, 2013 18:23 >>> *To:* LLVMdev at cs.uiuc.edu >>> *Cc:* Raoux, Thomas F >>> *Subject:* [LLVMdev] Mixing noalias and regular arguments >>> >>> Hi all, >>> >>> I'm trying to understand the semantics of noalias arguments, and I'm >>> not entirely sure I got it correctly. >>> >>> To the best of my understanding, if an argument is declared noalias, >>> "This indicates that pointer values based on the argument do not >>> alias pointer values which are not based on it" implies, among other >>> things, that it cannot alias any other argument, even if that >>> argument is NOT declared noalias. >>> >>> However, currently, BasicAliasAnalysis doesn't recognize this case >>> explicitly. Sometimes it will work for other reasons (e.g. if it >>> knows the other argument does not get captured), but it's relatively >>> easy to get circumstances where the result is MayAlias. >>> >>> I'm attaching a patch that addresses this. >>> >>> Can anyone offer an opinion on the basic issue and, assuming this is >>> the desired behavior, on the patch? >>> >>> Thanks, >>> >>> Michael >> --------------------------------------------------------------------- >> Intel Israel (74) Limited >> >> This e-mail and any attachments may contain confidential material for >> the sole use of the intended recipient(s). Any review or distribution >> by others is strictly prohibited. If you are not the intended >> recipient, please contact the sender and delete all copies. > > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > >