[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