Richard Sandiford - Re: [RTL, ColdFire 24/63] Add support for a MODE_INDEX_REG_CLASS macro (original) (raw)

This is the mail archive of the gcc-patches@gcc.gnu.orgmailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

In another thread, Jeff wrote:

Jeff Law law@redhat.com writes:

Richard Sandiford richard@codesourcery.com writes:

OK. Just so we're on the same page, what's the blocking issue here? I've tried to answer the PA question. I haven't had a chance to look at your response yet. I've been on the road some the last couple weeks and my GCC time is very limited these days.

I think I misled you into thinking there was a new message about this. There wasn't. I was actually referring to parts of the thread you'd already responded to.

To recap, your objection was that the m68k port should instead handle indexed registers in the same way as the PA port. So, to make things easier for me, let's call the two alternatives the "m68k way" and the "PA way".

The ColdFire FPU has no instructions for moving an indexed address into an FP register. This leads to the crucial point:

(a) the move patterns have no alternatives that move an indexed address into an FP register.

and, also crucial:

(b) (a) will be true for both the "m68k way" and the "PA way".

The bug is about addresses generated by find_reloads_address. If double_reg_address_ok is true, find_reloads_address will reload an out-of-range offset into an index register. So let's say we have:

(reg:SF pseudo)

before reload, and this register is constrained to be an FP register. And let's say that PSEUDO is not allocated a hard register, and instead gets spilled to:

(mem:SF (plus:SI fp offset))

where OFFSET is out of range. In this sort of case, find_reloads_address will decide to legitimize the address by reloading offset into an index register. find_reloads then reloads the whole MEM into a floating-point register F.

Reload is doing nothing wrong here. However:

F <- (mem:SF (plus:SI fp index))

is not going to match any move instruction. Like I say, this will be true for the both the PA way and the m68k way, becuase of (b). The PA way is not in itself a way of fixing this problem.

Referring back to the thread:

Richard Sandiford richard@codesourcery.com writes:

Jeff, just to confirm... Richard Sandiford richard@codesourcery.com writes:

Jeffrey Law law@redhat.com writes:

On Wed, 2007-01-10 at 21:27 +0000, Richard Sandiford wrote:

Right, that describes the ISA restrictions. But this patch isn't really about the ISA restrictions per se. The m68k port makes a pragmatic decision to forbid indexed addressing modes for all SFmode and DFmode MEMs if TARGET_COLDFIRE_FPU, on the basis that the vast majority of memory accesses will be in FPU instructions. We then expose the necessary addressing code to the pre-reload optimisers and register allocators, rather than leaving reload to find scratch address registers from somewhere. Although I didn't make that decision (it's what current mainline does), I can well imagine that it leads to better code.

In other words, this is entirely an internal gcc thing. The current definition of INDEX_REG_CLASS tricks reload into thinking that indexed addressing modes are acceptable SFmode and DFmode memory_operands, which they aren't. It will then use such memories in copy-in and copy-out reload instructions. The fundamental problem is that the m68k port is lying and you're hacking up reload to deal with the m68k version of the lie.

I disagree that this patch is a hack. I think the only real liar here is INDEX_REG_CLASS, which says that something is a valid index register even when GO_IF_LEGITIMATE_ADDRESS forbids it. I think the situation after the patch is self-consistent and the correct approach to take (in terms of what addresses are and aren't acceptable).

FWIW, the PA port lies too, but all the hair of the lie is buried in the backend. You might review how the PA handles this situation since they are quite similar (basically reversed -- the PA has full indexed addressing modes for loads/stores using FP registers, but not for integer registers.

To recap, the original problem was that reload creates copy-in reloads from (or copy-out reloads to) invalid indexed addresses. This can happen when a spill slot MEM has an out-of-range offset, for example.

If I've understood correctly, the PA port only avoids this problem for SImode MEMs because it defines LEGITIMIZE_RELOAD_ADDRESS. L_R_A deals with out-of-range offsets by adding the high part of the offset to the base and then adding the left-over offset. I'm sure that this definition was added purely for the sake of optimisation, but it has the side-effect of stopping reload from loading out-of-range offsets into index registers. Would everything still work without L_R_A? If not, that seems a little suspect. I'd always been told that L_R_A should only be used for optimisation, and that it was wrong to rely on it for correctness.

This started a minithread about whether it was OK to rely on L_R_A for correctness. I think you (Jeff) and I are in the "no" camp and Ian's in the "yes" camp.

I was wrong about the PA relying on it for correctness though. The m68k problem doesn't happen on the PA because double_reg_address_ok is false on the PA. That's because the PA doesn't allow disp(base,index) QImode addresses whereas m68k does. However, if the PA did allow disp(base,index) QImode addresses, it would be relying on L_R_A for correctness.

Thus something like L_R_A would fix the m68k problem. We could intercept all out-of-range FPmode offsets and legitimize them in the same sort of way as the PA, short-circuiting the normal reload code. However, I see this a hack, and wrong in principlem because I don't think L_R_A should be used for correctness like this.

In summary, I think this patch is solving a problem that does not occur on the PA, and that the PA approach doesn't solve.

I'll quote the rest of the message because I still stand by it. I don't really have anything to add to it though.

The wider issue seems to be: should we either

(a) allow indexed addresses for all modes and use constraints and secondary reloads to handle the cases where they aren't allowed or

(b) prevent the use of index registers for modes if the vast majority of MEMs in those modes are likely to not be index registers.

(a) is what PA does and (b) is what m68k does. I think (b) is the right choice for the reason outlined in the quote above: if we allow indexed MEMs before reload, it's up to reload to convert indexed MEMs into legitimate addresses. To do that it needs some scratch register, which it will pick in an ad-hoc way. It's better to expose the scratch register before reload if possible, and that's what (b) does. Note that the effect is likely to be more obvious on m68k than PA because the m68k port sometimes has only 5 allocatable address registers (after you've taken away the stack pointer, frame pointer and PIC register).

Note that the m68k port already deals with SImode indexed MEMs in the same way as the PA. It accepts indexed addresses and use constraints to restrict the choice.

Richard

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]