Regression with r200947: [CodeGenPrepare] Move away sign extensions that get in the way of addressing mode.

[Tom Stellard]

On Thu, Feb 13, 2014 at 09:39:52AM -0800, Quentin Colombet wrote:
> Hi Tom,
> On Feb 13, 2014, at 8:22 AM, Tom Stellard <tom at stellard.net> wrote:
> 
> > On Wed, Feb 12, 2014 at 02:55:57PM -0800, Quentin Colombet wrote:
> >> 
> >> On Feb 12, 2014, at 2:07 PM, Tom Stellard <tom at stellard.net> wrote:
> >> 
> >>> On Wed, Feb 12, 2014 at 01:43:25PM -0800, Quentin Colombet wrote:
> >>>> Hi Tom,
> >>>> 
> >>>> I think you find a bug in TargetLoweringBase::isLegalAddressingMode.
> >>>> 
> >>>> Indeed, TargetLoweringBase::isLegalAddressingMode (which is what it is called in your case), says that 3*r + r is a valid addressing mode.
> >>>> Basically, this returns true for every scale that is not 0, 1, and 2. I believe it misses a default case in the switch statement that return false.
> >>>> 
> >>>> Alternatively, you could have overloaded this function for your target, to be sure it accepts only what it should.
> >>>> 
> >>>> Assuming you fixes this problem, the promotion would still be performed because in that case the transformation is neutral (i.e., we do not create new instructions).
> >>>> The difference is: instead of selecting:
> >>>> %in + %sext
> >>>> we would select:
> >>>> %in + %promoted_mul
> >>>> 
> >>>> That said, the mul 64bit is not a legal operation on your platform (nor is mul i32), thus, I could drop the change if we did not fold the instruction into the addressing mode when the operation is not legal.
> >>>> 
> >>> 
> >>> I think it makes sense to avoid the promotion if the result can't be
> >>> folded into the addressing mode.
> >> Well, that is a bit more complex than this.
> >> When it is neutral, it may still be a good idea to perform the transformation because it may expose more simplifications (like sext + load).
> >> However, I would prefer the promotion not to be smart about that, at least for now.
> >> 
> >> Therefore, I still think that checking for an  operation to be legal or not may still be a viable approach.
> >> 
> >> 
> >>> How will you determine whether or not an
> >>> operation is legal?
> >> TLI.isOperationLegalOrCustom?
> >> 
> > 
> > Ok, I didn't realize IR passes had access to this.
> > 
> >> See the attached patch for an example of implementation.
> >> 
> >> If this serves your purposes, let me know when you fixed the isLegalAddressingMode and I will commit this patch with your test case.
> >> 
> > 
> > I just sent a patch to the list to fix isLegalAddressingMode.  I
> > included the test case with that patch and marked it as XFAIL.  With
> > this patch and your patch everything works fine.
> Great!
> I’ll wait for that patch to land before including this one.
> 

I just committed this patch.  Make sure to remove XFAIL from the test case when you
commit yours.  Thanks for fixing this.

-Tom

> Thanks,
> -Quentin
> 
> > 
> > Thanks,
> > Tom
> > 
> >> As it is the matched thinks the mul is folded anyway, making the proposed tuning useless.
> >> 
> >> Thanks,
> >> -Quentin
> >> 
> >>> 
> >>> -Tom
> >>> 
> >>>> Thanks for the test case.
> >>>> -Quentin
> >>>> 
> >>>> On Feb 12, 2014, at 12:58 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> >>>> 
> >>>>> Sure, I’m looking into it.
> >>>>> 
> >>>>> -Quentin
> >>>>> 
> >>>>> On Feb 12, 2014, at 12:03 PM, Tom Stellard <tom at stellard.net> wrote:
> >>>>> 
> >>>>>> Hi Quentin,
> >>>>>> 
> >>>>>> I've discovered a regression with this commit, please see the attached test case.
> >>>>>> In this case, CodeGenPrepare is promoting the mul in a sext + mul pattern even
> >>>>>> though the addressing mode it is creating isn't legal.
> >>>>>> 
> >>>>>> One interesting thing about this test case is that if you remove the
> >>>>>> nsw from the mul instruction, then the incorrect transform does not
> >>>>>> take place.  I'm not sure why this matters.  Would you mind taking a look?
> >>>>>> 
> >>>>>> Thanks,
> >>>>>> Tom
> >>>>>> <codegen-prepare-addrmode-sext.ll>
> >>>>> 
> >>>>> _______________________________________________
> >>>>> llvm-commits mailing list
> >>>>> llvm-commits at cs.uiuc.edu
> >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>