[PATCH] Minor fix to StackColoring to avoid needlessly clearing mem operands.

The following code snippet taken from StackColoring::remapInstructions clears a mem operand if it can’t guarantee whether the memoperand’s underlying value aliases with the merged allocas:

// Update the MachineMemOperand to use the new alloca.
522 for (MachineInstr::mmo_iterator MM = I->memoperands_begin(),

// Climb up and find the original alloca.
532 V = GetUnderlyingObject(V);
533 // If we did not find one, or if the one that we found is not in our
534 // map, then move on.
535 if (!V || !isa(V)) {
536 // Clear mem operand since we don’t know for sure that it doesn’t
537 // alias a merged alloca.
538 MMO->setValue(0);
539 continue;
540 }

The attached patch makes the code above less conservative. It avoids clearing a mem operand if its underlying value is a PseudoSourceValue and PseudoSourceValue::isConstant returns true. This enables MachineLICM to hoist loads from GOT out of a loop (see test case in stackcoloring-test.patch).

Please review.

Question: Why does it need to clear a mem operand if the underlying object is not an AllocaInst? I am not sure if I understand the reason for this.

stackcoloring.patch (917 Bytes)

stackcoloring-test.patch (1.39 KB)

Hi Akira, did anything happen with this patch?

Ciao, Duncan.

Hi Duncan,

No, it hasn’t been reviewed yet.

I missed the original email with the patch. Do you mind sending it again ?

Thanks,
Nadav

This is the email I sent last week.

stackcoloring.patch (917 Bytes)

stackcoloring-test.patch (1.39 KB)

The patch LGTM. The StackColoring patch is still too conservative and I am consulting with Jakob and Andy about possible solutions.

Thanks, I’ll commit the patch shortly.

This is the email I sent last week.

From: Akira Hatanaka <ahatanak@gmail.com>
Date: Wed, May 8, 2013 at 7:04 PM
Subject: [PATCH] Minor fix to StackColoring to avoid needlessly clearing mem operands.
To: LLVM Developers Mailing List <llvmdev@cs.uiuc.edu>

The following code snippet taken from StackColoring::remapInstructions clears a mem operand if it can’t guarantee whether the memoperand’s underlying value aliases with the merged allocas:

// Update the MachineMemOperand to use the new alloca.
522 for (MachineInstr::mmo_iterator MM = I->memoperands_begin(),

// Climb up and find the original alloca.
532 V = GetUnderlyingObject(V);
533 // If we did not find one, or if the one that we found is not in our
534 // map, then move on.
535 if (!V || !isa(V)) {
536 // Clear mem operand since we don’t know for sure that it doesn’t
537 // alias a merged alloca.
538 MMO->setValue(0);
539 continue;
540 }

The attached patch makes the code above less conservative. It avoids clearing a mem operand if its underlying value is a PseudoSourceValue and PseudoSourceValue::isConstant returns true. This enables MachineLICM to hoist loads from GOT out of a loop (see test case in stackcoloring-test.patch).

Please review.

Question: Why does it need to clear a mem operand if the underlying object is not an AllocaInst? I am not sure if I understand the reason for this.

See llvm.org/pr14090 and test/CodeGen/X86/pr14090.ll.

The fix is super-conservative. An optimization would be to guarantee that StackColoring and ScheduleDAGInstrs use exactly the same API for underlying objects. Even with that fix though, the alias info could be incorrect because we remap field accesses to the alloca base, so base-offset disambiguation could fail.

The only way to make this safe in general for IR-level alias analysis is to transform the IR, which would naturally be done with an IR pass.

-Andy