Fixed spill stack objects are mutable

[kparzysz]

Summary

CreateFixedSpillStackObject marked the created objects as immutable. This is an error, since spill stack objects are stored to, while immutable objects are assumed to never change during the function execution.

[reames]

At least per documentation, spill slots should be created via CreateSpillStackObject. I think you may be fixing the wrong bug. Even if not silently disabling all optimization for existing callers is not acceptable.

[hfinkel]

In D24039#529406, @reames wrote:

At least per documentation, spill slots should be created via CreateSpillStackObject. I think you may be fixing the wrong bug. Even if not silently disabling all optimization for existing callers is not acceptable.

I'm not quite sure that's right: Some ABIs have fixed-offset spill slots (e.g. PowerPC). Calling a spill slot immutable seems odd anyway, given that, by definition, you store into it. Also, they have isAliased == false regardless.

[kparzysz]

What documentation are you referring to?

Second of all, the rest of your comment sounds strange given my description of the problem. It is exactly that optimizations taking advantage of the "immutable" flag should not be taking place, given that these slots are not immutable.

[reames]

In D24039#529495, @kparzysz wrote:

What documentation are you referring to?

In the same header:
/// Create a new statically sized stack object that represents a spill slot,

/// returning a nonnegative identifier to represent it.
int CreateSpillStackObject(uint64_t Size, unsigned Alignment);

I also confirmed that the register allocator (via VirtRegMap::createSpillSlot) does use this interface.

Second of all, the rest of your comment sounds strange given my description of the problem. It is exactly that optimizations taking advantage of the "immutable" flag should not be taking place, given that these slots are not immutable.

My point is that while *spill slots* are clearly not immutable, there are other stack slots created by the same interface you modified which are. Disabling optimization of those slots is clearly not okay.

[reames]

In D24039#529494, @hfinkel wrote:

In D24039#529406, @reames wrote:

At least per documentation, spill slots should be created via CreateSpillStackObject. I think you may be fixing the wrong bug. Even if not silently disabling all optimization for existing callers is not acceptable.

I'm not quite sure that's right: Some ABIs have fixed-offset spill slots (e.g. PowerPC). Calling a spill slot immutable seems odd anyway, given that, by definition, you store into it. Also, they have isAliased == false regardless.

Hal, I think we have a terminology problem here. To me a "spill slot" is one created by the register allocator when spilling vregs. Are you saying that PowerPC has preassigned offsets which the register allocator must use? Or are you referring to something else entirely?

[kparzysz]

In D24039#529742, @reames wrote:

In the same header:
/// Create a new statically sized stack object that represents a spill slot,

/// returning a nonnegative identifier to represent it.
int CreateSpillStackObject(uint64_t Size, unsigned Alignment);

That same header has

/// Create a spill slot at a fixed location on the stack.
/// Returns an index with a negative value.
int CreateFixedSpillStackObject(uint64_t Size, int64_t SPOffset);

My point is that while *spill slots* are clearly not immutable, there are other stack slots created by the same interface you modified which are. Disabling optimization of those slots is clearly not okay.

Spill slot is a stack slot that is not aliased to anything else. Fixed spill slot is a spill slot with a specified offset (i.e. its position in the frame is fixed).

Edit: My change modifies the CreateFixedSpillStackObject. It affects a subset of all spill slots, not even all of them: the inclusion that your concern states is reversed.

[hfinkel]

In D24039#529748, @reames wrote:

In D24039#529494, @hfinkel wrote:

In D24039#529406, @reames wrote:

At least per documentation, spill slots should be created via CreateSpillStackObject. I think you may be fixing the wrong bug. Even if not silently disabling all optimization for existing callers is not acceptable.

I'm not quite sure that's right: Some ABIs have fixed-offset spill slots (e.g. PowerPC). Calling a spill slot immutable seems odd anyway, given that, by definition, you store into it. Also, they have isAliased == false regardless.

Hal, I think we have a terminology problem here. To me a "spill slot" is one created by the register allocator when spilling vregs. Are you saying that PowerPC has preassigned offsets which the register allocator must use? Or are you referring to something else entirely?

Fair enough. Yes, PowerPC has these, for callee-saved-register saving, and CreateFixedSpillStackObject is not used for those. CreateFixedSpillStackObject is only used by Hexagon and by x86. x86 uses this function in X86FrameLowering::assignCalleeSavedSpillSlots for assigning CSR spill slots. Hexagon seems to do the same (in HexagonFrameLowering::assignCalleeSavedSpillSlots).

My understanding is that this matters only when you have post-RA scheduling. Without post-RA scheduling, these slots are "effectively immutable", since they're stored to in the prologue and loaded in the epilogue, and that code isn't re-scheduled. With post-RA scheduling, this code might be re-scheduled, and it is important to actually consider whether the stores in the prologue might write to the slots being read by the epilogue code. This consideration won't happen if the slot is "immutable" (because, if it is immutable, then the store must be irrelevant). Krzysztof, did I understand the situation correctly?

If my understanding is correct, this seems like the right fix. It seems unlikely to affect performance on targets other than Hexagon and non-server-class x86 cores (i.e. Atom) where post-RA scheduling might be enabled.

[reames]

In D24039#529770, @kparzysz wrote:

In D24039#529742, @reames wrote:

In the same header:
/// Create a new statically sized stack object that represents a spill slot,

/// returning a nonnegative identifier to represent it.
int CreateSpillStackObject(uint64_t Size, unsigned Alignment);

That same header has

/// Create a spill slot at a fixed location on the stack.
/// Returns an index with a negative value.
int CreateFixedSpillStackObject(uint64_t Size, int64_t SPOffset);

My point is that while *spill slots* are clearly not immutable, there are other stack slots created by the same interface you modified which are. Disabling optimization of those slots is clearly not okay.

Spill slot is a stack slot that is not aliased to anything else. Fixed spill slot is a spill slot with a specified offset (i.e. its position in the frame is fixed).

Gah! I reread your patch and realized I'd misread it originally. I thought you were changing "CreateFixedObject" not "CreateFixedSpillStackObject". All of my comments are off base, apologies for misleading the discussion.

[hfinkel]

In D24039#529780, @reames wrote:

In D24039#529770, @kparzysz wrote:

In D24039#529742, @reames wrote:

In the same header:
/// Create a new statically sized stack object that represents a spill slot,

/// returning a nonnegative identifier to represent it.
int CreateSpillStackObject(uint64_t Size, unsigned Alignment);

That same header has

/// Create a spill slot at a fixed location on the stack.
/// Returns an index with a negative value.
int CreateFixedSpillStackObject(uint64_t Size, int64_t SPOffset);

My point is that while *spill slots* are clearly not immutable, there are other stack slots created by the same interface you modified which are. Disabling optimization of those slots is clearly not okay.

Spill slot is a stack slot that is not aliased to anything else. Fixed spill slot is a spill slot with a specified offset (i.e. its position in the frame is fixed).

Gah! I reread your patch and realized I'd misread it originally. I thought you were changing "CreateFixedObject" not "CreateFixedSpillStackObject". All of my comments are off base, apologies for misleading the discussion.

Good ;) -- In that case, this LGTM.

[reames]

In D24039#529774, @hfinkel wrote:

In D24039#529748, @reames wrote:

In D24039#529494, @hfinkel wrote:

In D24039#529406, @reames wrote:

At least per documentation, spill slots should be created via CreateSpillStackObject. I think you may be fixing the wrong bug. Even if not silently disabling all optimization for existing callers is not acceptable.

I'm not quite sure that's right: Some ABIs have fixed-offset spill slots (e.g. PowerPC). Calling a spill slot immutable seems odd anyway, given that, by definition, you store into it. Also, they have isAliased == false regardless.

Hal, I think we have a terminology problem here. To me a "spill slot" is one created by the register allocator when spilling vregs. Are you saying that PowerPC has preassigned offsets which the register allocator must use? Or are you referring to something else entirely?

Fair enough. Yes, PowerPC has these, for callee-saved-register saving, and CreateFixedSpillStackObject is not used for those. CreateFixedSpillStackObject is only used by Hexagon and by x86. x86 uses this function in X86FrameLowering::assignCalleeSavedSpillSlots for assigning CSR spill slots. Hexagon seems to do the same (in HexagonFrameLowering::assignCalleeSavedSpillSlots).

My understanding is that this matters only when you have post-RA scheduling. Without post-RA scheduling, these slots are "effectively immutable", since they're stored to in the prologue and loaded in the epilogue, and that code isn't re-scheduled. With post-RA scheduling, this code might be re-scheduled, and it is important to actually consider whether the stores in the prologue might write to the slots being read by the epilogue code. This consideration won't happen if the slot is "immutable" (because, if it is immutable, then the store must be irrelevant). Krzysztof, did I understand the situation correctly?

If my understanding is correct, this seems like the right fix. It seems unlikely to affect performance on targets other than Hexagon and non-server-class x86 cores (i.e. Atom) where post-RA scheduling might be enabled.

Hal, thanks for the context. As I replied separately, I'd misread the change, but your explanation provided a good amount of context which is great to understand anyways.

[kparzysz]

In D24039#529780, @reames wrote:

Gah! I reread your patch and realized I'd misread it originally. I thought you were changing "CreateFixedObject" not "CreateFixedSpillStackObject". All of my comments are off base, apologies for misleading the discussion.

No problem. :)

[kparzysz]

In D24039#529774, @hfinkel wrote:

Krzysztof, did I understand the situation correctly?

Yes, exactly. I'd only add that the problem can also occur during packetization (as a matter of fact, that's where I discovered it).

[kparzysz]

Committed in r280244.

llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h

public:
   /// All fixed objects should be created before other objects are created for
   /// efficiency. By default, fixed objects are not pointed to by LLVM IR
   /// values. This returns an index with a negative value.
   int CreateFixedObject(uint64_t Size, int64_t SPOffset, bool Immutable,
                         bool isAliased = false);
 
   /// Create a spill slot at a fixed location on the stack.
   /// Returns an index with a negative value.
-  int CreateFixedSpillStackObject(uint64_t Size, int64_t SPOffset);
+  int CreateFixedSpillStackObject(uint64_t Size, int64_t SPOffset,
+                                  bool Immutable = false);
 
   /// Returns true if the specified index corresponds to a fixed stack object.
   bool isFixedObjectIndex(int ObjectIdx) const {
     return ObjectIdx < 0 && (ObjectIdx >= -(int)NumFixedObjects);
   }
 
   /// Returns true if the specified index corresponds
   /// to an object that might be pointed to by an LLVM IR value.

llvm/trunk/lib/CodeGen/MachineFunction.cpp

Objects.insert(Objects.begin(), StackObject(Size, Align, SPOffset, Immutable,
                                               /*isSS*/   false,
                                               /*Alloca*/ nullptr, isAliased));
   return -++NumFixedObjects;
 }
 
 /// Create a spill slot at a fixed location on the stack.
 /// Returns an index with a negative value.
 int MachineFrameInfo::CreateFixedSpillStackObject(uint64_t Size,
-                                                  int64_t SPOffset) {
+                                                  int64_t SPOffset,
+                                                  bool Immutable) {
   unsigned Align = MinAlign(SPOffset, ForcedRealign ? 1 : StackAlignment);
   Align = clampStackAlignment(!StackRealignable, Align, StackAlignment);
-  Objects.insert(Objects.begin(), StackObject(Size, Align, SPOffset,
-                                              /*Immutable*/ true,
+  Objects.insert(Objects.begin(), StackObject(Size, Align, SPOffset, Immutable,
                                               /*isSS*/ true,
                                               /*Alloca*/ nullptr,
                                               /*isAliased*/ false));
   return -++NumFixedObjects;
 }
 
 BitVector MachineFrameInfo::getPristineRegs(const MachineFunction &MF) const {
   const TargetRegisterInfo *TRI = MF.getSubtarget().getRegisterInfo();

llvm/trunk/test/CodeGen/Hexagon/fixed-spill-mutable.ll

+; RUN: llc -march=hexagon < %s | FileCheck %s
+
+; The early return is predicated, and the save-restore code is mixed together:
+; {
+;   p0 = cmp.eq(r0, #0)
+;   if (p0.new) r17:16 = memd(r29 + #0)
+;   memd(r29+#0) = r17:16
+; }
+; {
+;   if (p0) dealloc_return
+; }
+; The problem is that the load will execute before the store, clobbering the
+; pair r17:16.
+;
+; Check that the store and the load are not in the same packet.
+; CHECK: memd{{.*}} = r17:16
+; CHECK: }
+; CHECK: r17:16 = memd
+; CHECK-LABEL: LBB0_1:
+
+target triple = "hexagon"
+
+%struct.0 = type { i8*, %struct.1*, %struct.2*, %struct.0*, %struct.0* }
+%struct.1 = type { [60 x i8], i32, %struct.1* }
+%struct.2 = type { i8, i8, i8, i8, %union.anon }
+%union.anon = type { %struct.3* }
+%struct.3 = type { %struct.3*, %struct.2* }
+
+@var = external hidden unnamed_addr global %struct.0*, align 4
+
+declare void @bar(i8*, i32) local_unnamed_addr #0
+
+define void @foo() local_unnamed_addr #1 {
+entry:
+  %.pr = load %struct.0*, %struct.0** @var, align 4, !tbaa !1
+  %cmp2 = icmp eq %struct.0* %.pr, null
+  br i1 %cmp2, label %while.end, label %while.body.preheader
+
+while.body.preheader:                             ; preds = %entry
+  br label %while.body
+
+while.body:                                       ; preds = %while.body.preheader, %while.body
+  %0 = phi %struct.0* [ %4, %while.body ], [ %.pr, %while.body.preheader ]
+  %right = getelementptr inbounds %struct.0, %struct.0* %0, i32 0, i32 4
+  %1 = bitcast %struct.0** %right to i32*
+  %2 = load i32, i32* %1, align 4, !tbaa !5
+  %3 = bitcast %struct.0* %0 to i8*
+  tail call void @bar(i8* %3, i32 20) #1
+  store i32 %2, i32* bitcast (%struct.0** @var to i32*), align 4, !tbaa !1
+  %4 = inttoptr i32 %2 to %struct.0*
+  %cmp = icmp eq i32 %2, 0
+  br i1 %cmp, label %while.end.loopexit, label %while.body
+
+while.end.loopexit:                               ; preds = %while.body
+  br label %while.end
+
+while.end:                                        ; preds = %while.end.loopexit, %entry
+  ret void
+}
+
+attributes #0 = { optsize }
+attributes #1 = { nounwind optsize }
+
+!1 = !{!2, !2, i64 0}
+!2 = !{!"any pointer", !3, i64 0}
+!3 = !{!"omnipotent char", !4, i64 0}
+!4 = !{!"Simple C/C++ TBAA"}
+!5 = !{!6, !2, i64 16}
+!6 = !{!"0", !2, i64 0, !2, i64 4, !2, i64 8, !2, i64 12, !2, i64 16}