Björn Steinbrink via llvm-dev
2017-Jan-31 10:09 UTC
[llvm-dev] Folding zext from i1 into PHI nodes with only zwo incoming values.
Hi Sanjay, 2017-01-30 22:22 GMT+01:00 Sanjay Patel <spatel at rotateright.com>:> My minimal patch idea is to ease the restriction in ShouldChangeType > because i1 is special. I tried the patch below, and it works on the > example...and nothing in 'make check' failed. :) >Yeah, that would work for me as well, I just wasn't sure about the implications that has. Simply making FoldPHIArgOpIntoPHI act like FoldPHIArgZextsIntoPHI seemed like the safer option to me, but I wanted feedback on it before creating a PR. Do you want to go ahead with that minimal approach and create a PR yourself? Björn> Index: lib/Transforms/InstCombine/InstructionCombining.cpp > ==================================================================> --- lib/Transforms/InstCombine/InstructionCombining.cpp (revision > 293485) > +++ lib/Transforms/InstCombine/InstructionCombining.cpp (working copy) > @@ -88,12 +88,12 @@ > > /// Return true if it is desirable to convert an integer computation from > a > /// given bit width to a new bit width. > -/// We don't want to convert from a legal to an illegal type for example > or from > -/// a smaller to a larger illegal type. > +/// We don't want to convert from a legal to an illegal type or from a > smaller > +/// to a larger illegal type. Width of '1' is always treated as a legal > type. > bool InstCombiner::ShouldChangeType(unsigned FromWidth, > unsigned ToWidth) const { > - bool FromLegal = DL.isLegalInteger(FromWidth); > - bool ToLegal = DL.isLegalInteger(ToWidth); > + bool FromLegal = FromWidth == 1 ? true : DL.isLegalInteger(FromWidth); > + bool ToLegal = ToWidth == 1 ? true : DL.isLegalInteger(ToWidth); > > // If this is a legal integer from type, and the result would be an > illegal > // type, don't do the transformation. > @@ -110,7 +110,7 @@ > > /// Return true if it is desirable to convert a computation from 'From' > to 'To'. > /// We don't want to convert from a legal to an illegal type for example > or from > -/// a smaller to a larger illegal type. > +/// a smaller to a larger illegal type. i1 is always treated as a legal > type. > bool InstCombiner::ShouldChangeType(Type *From, Type *To) const { > assert(From->isIntegerTy() && To->isIntegerTy()); >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170131/38a0e09a/attachment.html>
Sanjay Patel via llvm-dev
2017-Jan-31 15:59 UTC
[llvm-dev] Folding zext from i1 into PHI nodes with only zwo incoming values.
On Tue, Jan 31, 2017 at 3:09 AM, Björn Steinbrink <bsteinbr at gmail.com> wrote:> Hi Sanjay, > > 2017-01-30 22:22 GMT+01:00 Sanjay Patel <spatel at rotateright.com>: > >> My minimal patch idea is to ease the restriction in ShouldChangeType >> because i1 is special. I tried the patch below, and it works on the >> example...and nothing in 'make check' failed. :) >> > > Yeah, that would work for me as well, I just wasn't sure about the > implications that has. Simply making FoldPHIArgOpIntoPHI act like > FoldPHIArgZextsIntoPHI seemed like the safer option to me, but I wanted > feedback on it before creating a PR. Do you want to go ahead with that > minimal approach and create a PR yourself? >Your idea probably has the minimal impact while still fixing your problem case...although the relationships between FoldPHIArgOpIntoPHI / FoldPHIArgZextsIntoPHI / FoldOpIntoPhi are confusing. I think a patch for ShouldChangeType makes sense on its own (and makes any direct changes to the FoldPHI* functions unnecessary for your example). I'll try some more experiments with that patch to look for unintended consequences. When you say "PR", do you mean a bugzilla report or a patch proposal on Phab?> > > Björn > > >> Index: lib/Transforms/InstCombine/InstructionCombining.cpp >> ==================================================================>> --- lib/Transforms/InstCombine/InstructionCombining.cpp (revision >> 293485) >> +++ lib/Transforms/InstCombine/InstructionCombining.cpp (working copy) >> @@ -88,12 +88,12 @@ >> >> /// Return true if it is desirable to convert an integer computation >> from a >> /// given bit width to a new bit width. >> -/// We don't want to convert from a legal to an illegal type for example >> or from >> -/// a smaller to a larger illegal type. >> +/// We don't want to convert from a legal to an illegal type or from a >> smaller >> +/// to a larger illegal type. Width of '1' is always treated as a legal >> type. >> bool InstCombiner::ShouldChangeType(unsigned FromWidth, >> unsigned ToWidth) const { >> - bool FromLegal = DL.isLegalInteger(FromWidth); >> - bool ToLegal = DL.isLegalInteger(ToWidth); >> + bool FromLegal = FromWidth == 1 ? true : DL.isLegalInteger(FromWidth); >> + bool ToLegal = ToWidth == 1 ? true : DL.isLegalInteger(ToWidth); >> >> // If this is a legal integer from type, and the result would be an >> illegal >> // type, don't do the transformation. >> @@ -110,7 +110,7 @@ >> >> /// Return true if it is desirable to convert a computation from 'From' >> to 'To'. >> /// We don't want to convert from a legal to an illegal type for example >> or from >> -/// a smaller to a larger illegal type. >> +/// a smaller to a larger illegal type. i1 is always treated as a legal >> type. >> bool InstCombiner::ShouldChangeType(Type *From, Type *To) const { >> assert(From->isIntegerTy() && To->isIntegerTy()); >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170131/7b66db3c/attachment.html>
Björn Steinbrink via llvm-dev
2017-Jan-31 16:02 UTC
[llvm-dev] Folding zext from i1 into PHI nodes with only zwo incoming values.
2017-01-31 16:59 GMT+01:00 Sanjay Patel <spatel at rotateright.com>:> > > On Tue, Jan 31, 2017 at 3:09 AM, Björn Steinbrink <bsteinbr at gmail.com> > wrote: > >> Hi Sanjay, >> >> 2017-01-30 22:22 GMT+01:00 Sanjay Patel <spatel at rotateright.com>: >> >>> My minimal patch idea is to ease the restriction in ShouldChangeType >>> because i1 is special. I tried the patch below, and it works on the >>> example...and nothing in 'make check' failed. :) >>> >> >> Yeah, that would work for me as well, I just wasn't sure about the >> implications that has. Simply making FoldPHIArgOpIntoPHI act like >> FoldPHIArgZextsIntoPHI seemed like the safer option to me, but I wanted >> feedback on it before creating a PR. Do you want to go ahead with that >> minimal approach and create a PR yourself? >> > > Your idea probably has the minimal impact while still fixing your problem > case...although the relationships between FoldPHIArgOpIntoPHI / > FoldPHIArgZextsIntoPHI / FoldOpIntoPhi are confusing. > > I think a patch for ShouldChangeType makes sense on its own (and makes any > direct changes to the FoldPHI* functions unnecessary for your example). > I'll try some more experiments with that patch to look for unintended > consequences. >OK, I'll wait for your results then> When you say "PR", do you mean a bugzilla report or a patch proposal on > Phab? >I meant a patch proposal on Phab, I'm too used to GitHub terminology by now, sorry. Cheers, Björn> > >> >> >> Björn >> >> >>> Index: lib/Transforms/InstCombine/InstructionCombining.cpp >>> ==================================================================>>> --- lib/Transforms/InstCombine/InstructionCombining.cpp (revision >>> 293485) >>> +++ lib/Transforms/InstCombine/InstructionCombining.cpp (working >>> copy) >>> @@ -88,12 +88,12 @@ >>> >>> /// Return true if it is desirable to convert an integer computation >>> from a >>> /// given bit width to a new bit width. >>> -/// We don't want to convert from a legal to an illegal type for >>> example or from >>> -/// a smaller to a larger illegal type. >>> +/// We don't want to convert from a legal to an illegal type or from a >>> smaller >>> +/// to a larger illegal type. Width of '1' is always treated as a legal >>> type. >>> bool InstCombiner::ShouldChangeType(unsigned FromWidth, >>> unsigned ToWidth) const { >>> - bool FromLegal = DL.isLegalInteger(FromWidth); >>> - bool ToLegal = DL.isLegalInteger(ToWidth); >>> + bool FromLegal = FromWidth == 1 ? true : DL.isLegalInteger(FromWidth); >>> + bool ToLegal = ToWidth == 1 ? true : DL.isLegalInteger(ToWidth); >>> >>> // If this is a legal integer from type, and the result would be an >>> illegal >>> // type, don't do the transformation. >>> @@ -110,7 +110,7 @@ >>> >>> /// Return true if it is desirable to convert a computation from 'From' >>> to 'To'. >>> /// We don't want to convert from a legal to an illegal type for >>> example or from >>> -/// a smaller to a larger illegal type. >>> +/// a smaller to a larger illegal type. i1 is always treated as a legal >>> type. >>> bool InstCombiner::ShouldChangeType(Type *From, Type *To) const { >>> assert(From->isIntegerTy() && To->isIntegerTy()); >>> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170131/64798dc6/attachment.html>
Reasonably Related Threads
- Folding zext from i1 into PHI nodes with only zwo incoming values.
- Folding zext from i1 into PHI nodes with only zwo incoming values.
- Folding zext from i1 into PHI nodes with only zwo incoming values.
- Folding zext from i1 into PHI nodes with only zwo incoming values.
- always allow canonicalizing to 8- and 16-bit ops?