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

[Tom Stellard]

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.

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
>