[PATCH] D24039: Fixed spill stack objects are mutable

[Philip Reames via llvm-commits]

reames added a comment.

In https://reviews.llvm.org/D24039#529774, @hfinkel wrote:

> In https://reviews.llvm.org/D24039#529748, @reames wrote:
>
> > In https://reviews.llvm.org/D24039#529494, @hfinkel wrote:
> >
> > > In https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D24039