search for: shouldchangetype

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

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

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

...uction at the end. Björn 2017-01-30 20:20 GMT+01:00 Sanjay Patel <spatel at rotateright.com>: > I'm looking at a similar problem in: > https://reviews.llvm.org/D28625 > > Does that patch make any difference on the cases that you are looking at? > > Instead of avoiding ShouldChangeType with zext as a special-case opcode, > it might be better to treat i1 as a special-case type. There's no way to > avoid i1 in IR, so we might as well allow transforming to that type? > > I'm not sure yet, but there's a chance that change might induce problems > (infinite l...

Hi Björn and Daniel, FoldPHIArgOpIntoPHI() is for unary ops (casts), but it calls FoldPHIArgBinOpIntoPHI() for binops which takes care of almost everything else? 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. :) Index: lib/Transforms/InstCombine/InstructionCombining.cpp =================================================================== --- lib/Transforms/InstCombine/InstructionComb...

...we saw a optimization regression when a function got specialized and the PHI node only had two incoming values left. Since I'm not fully aware of any implications this might have, I wonder what is the right way to fix this? Looking at FoldPHIArgZextsIntoPHI, it seems that making the check for `ShouldChangeType` in FoldPHIArgOpIntoPHI conditional on the cast instruction not being a zext instruction. Does that sound right, or am I missing something here? Thanks Björn -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/201...

Thanks for the perf testing. I assume that DAG legalization is equipped to handle these cases fairly well, or someone would've complained by now... FWIW (and at least some of this can be blamed on me), instcombine already does the narrowing transforms without checking shouldChangeType() for binops like and/or/xor/udiv. The justification was that narrower ops are always better for bit-tracking. If we can eliminate casts, then that improves analysis even more and allows more follow-on transforms. If there are no objections here, I think you can post your patch for review on Phabr...

...ow_add(i8 %x, i8 %y) { %add = add i8 %x, %y ret i8 %add } But on a target that has 32-bit registers without explicit subregister ops, we don't do that transform because we avoid changing operations from a legal (as specified in the data-layout) width to an illegal width - see InstCombiner::shouldChangeType(). Should we make an exception to allow narrowing for the common cases of i8 and i16? In the motivating example from PR35875 ( https://bugs.llvm.org/show_bug.cgi?id=35875 ), an ARM target is stuck at 19 IR instructions: declare void @use4(i8, i8, i8, i8) define void @min_of_3_vals(i8 %x, i8 %y,...

...uld be, especially on architectures that are not Arm. What I can do though, is run some benchmarks and look at that results. Using this patch: --- a/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -150,6 +150,9 @@ bool InstCombiner::shouldChangeType(unsigned FromWidth, bool FromLegal = FromWidth == 1 || DL.isLegalInteger(FromWidth); bool ToLegal = ToWidth == 1 || DL.isLegalInteger(ToWidth); + if (FromLegal && ToWidth < FromWidth && (ToWidth == 8 || ToWidth == 16)) + return true; + // If this is a legal integer...

We use InstCombiner::ShouldChangeType() to prevent transforms to illegal integer types, but I'm not sure how that would apply to vector types. Ie, let's say v256 is a legal type in your example. DataLayout doesn't appear to specify what configurations of a 256-bit vector are legal, so I don't think we can currently use...

...> *To:* Rackover, Zvi <zvi.rackover at intel.com> > *Cc:* Friedman, Eli <efriedma at codeaurora.org>; llvm-dev < > llvm-dev at lists.llvm.org> > *Subject:* Re: [llvm-dev] IR canonicalization: shufflevector or vector > trunc? > > > > We use InstCombiner::ShouldChangeType() to prevent transforms to illegal > integer types, but I'm not sure how that would apply to vector types. > > Ie, let's say v256 is a legal type in your example. DataLayout doesn't > appear to specify what configurations of a 256-bit vector are legal, so I > don't th...

...; > Thanks for the perf testing. I assume that DAG legalization is equipped to > handle these cases fairly well, or someone would've complained by now... > > FWIW (and at least some of this can be blamed on me), instcombine already > does the narrowing transforms without checking shouldChangeType() for binops > like and/or/xor/udiv. The justification was that narrower ops are always > better for bit-tracking. If we can eliminate casts, then that improves > analysis even more and allows more follow-on transforms. > > If there are no objections here, I think you can post your p...

Right - I think that case looks like this for little endian: define <2 x i32> @zextshuffle(<2 x i16> %x) { %zext_shuffle = shufflevector <2 x i16> %x, <2 x i16> zeroinitializer, <4 x i32> <i32 0, i32 2, i32 1, i32 2> %bc = bitcast <4 x i16> %zext_shuffle to <2 x i32> ret <2 x i32> %bc } define <2 x i32> @zextvec(<2 x i16>