[PATCH] D115690: [SystemZ] Implement orderFrameObjects().

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa marked 2 inline comments as done.
jonpa added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:165
+  };
+  std::stable_sort(SortingObjects.begin(), IE, CmpD12);
+
----------------
uweigand wrote:
> Why two calls to `stable_sort` here?  It seems this could be done in a single call with an appropriate comparison routine.
Ah, I guess that's true. For readability I thought it was better to have them separate as the first sort is not relating to anything SystemZ specific.

I guess now that the patch only does one additional sort, it makes sense to merge the two comparators and remove the IE iterator.



================
Comment at: llvm/test/CodeGen/SystemZ/stackmap.ll:474
+  %metadata2 = alloca i8, i32 4, align 4
+  %metadata3 = alloca i16, i32 4, align 4
   call void (i64, i32, i8*, i32, ...) @llvm.experimental.patchpoint.void(i64 17, i32 6, i8* null, i32 0, i8* %metadata2, i16* %metadata3)
----------------
uweigand wrote:
> This change seems unexpected - do you understand where this is coming from?
hmm - sorry that must have gone undetected as I simplified the patch. The test actually passes with and without this test change. I think I probably needed to modify this when the alignments sorting was done previously. Removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115690/new/

https://reviews.llvm.org/D115690