[PATCH] TargetLowering: n * r where n > 2 should be an illegal addressing mode

Tom Stellard tom at stellard.net
Fri Feb 14 14:05:31 PST 2014


On Fri, Feb 14, 2014 at 10:46:29AM +0000, Richard Sandiford wrote:
> Tom Stellard <tom at stellard.net> writes:
> > On Thu, Feb 13, 2014 at 08:21:04AM -0800, Tom Stellard wrote:
> >> From: Tom Stellard <thomas.stellard at amd.com>
> >> 
> >
> > Please ignore this patch.  Scaled addressing modes are more
> > common than I though, so I don't think this is correct.
> 
> I agree there's a problem though.  I hit the same thing before r187497:
> 
>     [SystemZ] Implement isLegalAddressingMode()
>     
>     The loop optimizers were assuming that scales > 1 were OK.  I think this
>     is actually a bug in TargetLoweringBase::isLegalAddressingMode(),
>     since it seems to be trying to reject anything that isn't r+i or r+r,
>     but it has no default case for scales other than 0, 1 or 2.  Implementing
>     the hook for z means that z can no longer test any change there though.
> 
> The comment says:
> 
>   // Only support r+r,
> 
> so I think it really was trying to disallow scaled addresses.  The switch
> doesn't make much sense otherwise.  E.g. the gymnastics the scale==2 case
> goes through:
> 
>     if (AM.HasBaseReg || AM.BaseOffs)  // 2*r+r  or  2*r+i is not allowed.
>       return false;
>     // Allow 2*r as r+r.
>     break;
> 
> If scaled addresses are supported then the set of valid scales are going
> to be target-dependent and need a target-specific override, so TBH I think
> your patch is correct.  I was going to post the same thing originally but
> implementing the z hook meant I no longer had a testcase.
> 

Thanks for following up on this.  It does appear as if the original intention
was to disallow scaled addressing modes and the comments seem to back this up.
I have committed this as r201433.

-Tom



More information about the llvm-commits mailing list