Andrew Trick
2012-May-30 17:04 UTC
[LLVMdev] [llvm-commits] [llvm] r157649 - /llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp
Originally on llvm-commits. On May 30, 2012, at 8:48 AM, Nuno Lopes <nunoplopes at sapo.pt> wrote:>> This is probably fine for now. It's a relatively safe use of SCEVExpander and the least effort approach, but generally I would like to encourage people to solve local rewrite problems with IRBuilder, not SCEVExpander, and build useful utilities for that purpose. Just because you use SCEV doesn't mean you have to use SCEVExpander. SCEVExpander may try to do more rewriting than you expect--it can create an indefinite number of new instructions. In general, it can create new phis, reassociate GEPs, drop "inbounds" and create ugly GEPS. Things I really hate to see before LSR! It also makes strong assumptions about valid input that the caller has to check first and is unable to bailout when it hits a strange case. Supporting SCEVExpander also imposes limitations on SCEV that would be nice to remove some day. > > Well, here I'm not rewriting anything /per se/. I'm generating a whole new expression which represents the value of the index of a GEP in the last loop iteration relative to the base object. In case we are lucky, it will be just 'N-1', for example. Unless SCEV cannot prove that the value doesn't overflow, and then we get a complex expression (but nothing with PHIs, I guess).And hopefully nothing with GEPs. So it's probably safe in your case.>> As an alternative, you can find your GEP's underlying phi and grab the value from the preheader. SCEV will tell you the difference between getSCEVAtScope and the preheader's expression, hopefully in terms of a constant or value+constant, and you should be able to materialize an inbounds gep. If any of these steps fail to find a simple solution, you can easily bailout. I'm glossing over the issues, but it would be nice to have a utility like this outside or alongside current SCEVExpander. I think most of the code in SCEVExpander is good, I just don't think the current design is well suited for use outside LSR. > > So you are suggesting, as an optimization, that if SCEV returns a simple 'value+constant' solution, I can simply take the constant and verify statically if the GEP will overflow or not. Adding inbounds flag is not really in the scope of this pass. Isn't there any pass that will do that?You would have to do some pattern matching to make best use of existing values instead of the indiscriminate rewrite that SCEVExpander does. It's more work and maybe not worthwhile in your case. My response was mainly intended to warn other developers and have them go through the mental exercise: "what would I do if SCEVExpander didn't exist"... instead of instinctively using it as a general utility. -Andy -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120530/c91bd957/attachment.html>
Hal Finkel
2012-May-31 16:55 UTC
[LLVMdev] [llvm-commits] [llvm] r157649 - /llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp
On Wed, 30 May 2012 10:04:50 -0700 Andrew Trick <atrick at apple.com> wrote:> Originally on llvm-commits. > > On May 30, 2012, at 8:48 AM, Nuno Lopes <nunoplopes at sapo.pt> wrote: > >> This is probably fine for now. It's a relatively safe use of > >> SCEVExpander and the least effort approach, but generally I would > >> like to encourage people to solve local rewrite problems with > >> IRBuilder, not SCEVExpander, and build useful utilities for that > >> purpose. Just because you use SCEV doesn't mean you have to use > >> SCEVExpander. SCEVExpander may try to do more rewriting than you > >> expect--it can create an indefinite number of new instructions. In > >> general, it can create new phis, reassociate GEPs, drop "inbounds" > >> and create ugly GEPS. Things I really hate to see before LSR! It > >> also makes strong assumptions about valid input that the caller > >> has to check first and is unable to bailout when it hits a strange > >> case. Supporting SCEVExpander also imposes limitations on SCEV > >> that would be nice to remove some day.Andy, Out of curiosity, could you be more specific? What limitations are we talking about? Thanks again, Hal> > > > Well, here I'm not rewriting anything /per se/. I'm generating a > > whole new expression which represents the value of the index of a > > GEP in the last loop iteration relative to the base object. In case > > we are lucky, it will be just 'N-1', for example. Unless SCEV > > cannot prove that the value doesn't overflow, and then we get a > > complex expression (but nothing with PHIs, I guess). > > And hopefully nothing with GEPs. So it's probably safe in your case. > > >> As an alternative, you can find your GEP's underlying phi and grab > >> the value from the preheader. SCEV will tell you the difference > >> between getSCEVAtScope and the preheader's expression, hopefully > >> in terms of a constant or value+constant, and you should be able > >> to materialize an inbounds gep. If any of these steps fail to find > >> a simple solution, you can easily bailout. I'm glossing over the > >> issues, but it would be nice to have a utility like this outside > >> or alongside current SCEVExpander. I think most of the code in > >> SCEVExpander is good, I just don't think the current design is > >> well suited for use outside LSR. > > > > So you are suggesting, as an optimization, that if SCEV returns a > > simple 'value+constant' solution, I can simply take the constant > > and verify statically if the GEP will overflow or not. Adding > > inbounds flag is not really in the scope of this pass. Isn't there > > any pass that will do that? > > You would have to do some pattern matching to make best use of > existing values instead of the indiscriminate rewrite that > SCEVExpander does. It's more work and maybe not worthwhile in your > case. My response was mainly intended to warn other developers and > have them go through the mental exercise: "what would I do if > SCEVExpander didn't exist"... instead of instinctively using it as a > general utility. > > -Andy-- Hal Finkel Postdoctoral Appointee Leadership Computing Facility Argonne National Laboratory
Andrew Trick
2012-May-31 16:58 UTC
[LLVMdev] [llvm-commits] [llvm] r157649 - /llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp
On May 31, 2012, at 9:55 AM, Hal Finkel <hfinkel at anl.gov> wrote:>>>> case. Supporting SCEVExpander also imposes limitations on SCEV >>>> that would be nice to remove some day. > > Andy, > > Out of curiosity, could you be more specific? What limitations are we > talking about?The one I remember now is the inability to analyze inttoptr/ptrtoint. There are probably more though. -Andy -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120531/4600d815/attachment.html>
Possibly Parallel Threads
- [LLVMdev] [llvm-commits] [llvm] r157649 - /llvm/trunk/lib/Transforms/Scalar/BoundsChecking.cpp
- Request suggestions about how to remove redundencies caused by SCEV expansion fundementally
- Request suggestions about how to remove redundencies caused by SCEV expansion fundementally
- LSR/SCEV problem/question
- SCEVExpander and IRBuilder