[llvm-commits] [llvm] r146604 - in /llvm/trunk: lib/CodeGen/MachineSink.cpp test/CodeGen/ARM/2011-12-14-machine-sink.ll

[Jakob Stoklund Olesen]

On Dec 23, 2011, at 10:43 AM, Akira Hatanaka <ahatanak at gmail.com> wrote:

> The problem seems to be that InstructionStoresToFI iterates over
> MachineMemOperands to see if the instruction is a store to a
> particular frame object. It doesn't return true if there are no
> MachineMemOperands.
> 
> static bool InstructionStoresToFI(const MachineInstr *MI, int FI)
> {
>   for (MachineInstr::mmo_iterator o = MI->memoperands_begin(),
>          oe = MI->memoperands_end(); o != oe; ++o) {
>     if (!(*o)->isStore() || !(*o)->getValue())
>       continue;
> ...
> }

Yep, that's the bug. The function is only called for spill slots, though. They have memops in most targets. (now including Mips).

> It looks like the FI could be retrieved and checked by calling
> TII->isStoreToStackSlot(MI, FI) regardless of whether there are
> MachineMemOperands attached, but there might be a reason I don't see
> for examining the MachineMemOperands instead.

Spill slot accesses can be folded into other instructions on CISC architectures. The isStoreToStackSlot only recognizes pure spills.

However, if an instruction has a spill slot operand, and its mayStore method returns true, it should be assumed it can write the spill slot. Memory operands should only be used to enable optimizations, not to disable them.

Feel free to fix this bug.

/jakob

> 
> On Thu, Dec 22, 2011 at 5:16 PM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>> 
>> On Dec 22, 2011, at 10:10 AM, Akira Hatanaka wrote:
>> 
>>> It turns out that the failure was caused by a bug in
>>> MipsInstrInfo::storeRegToStackSlot and loadRegFromStackSlot. A
>>> MachineMemOperand was not being added to the generated instruction,
>>> which was causing MachineLICM to incorrectly determine it is safe to
>>> move a load out of a loop. So this wasn't a mis-compilation caused by
>>> a bug in Machine Sink or the register allocator.
>> 
>> Nice!
>> 
>> It does sound like an LICM bug, though. A load or store without any memory operands should be treated as volatile, aliases-everything.
>> 
>> /jakob
>>