[PATCH] D19924: Representing bundle locked groups as fragments in MCAssembler

[Derek Schuff via llvm-commits]

dschuff added a comment.

In general I like this direction too. Here are a few preliminary comments. Wrt mc-relax-all, we haven't completely replaced llc in PNaCl yet; I wouldn't necessarily be against removing mc-relax-all anyway though. We might want to consider how this would impact memory usage, and especially if it's an improvement over the previous "normal" case, then that's a no-brainer.


================
Comment at: lib/MC/MCAssembler.cpp:356
@@ +355,3 @@
+  while (Count > 0) {
+    uint64_t Amount = std::min(Count, (((Offset / BundleSize) + 1) * BundleSize) - Offset);
+    Count -= Amount;
----------------
I guess this code enforces the requirement that nop data not cross bundle boundaries? Can we add back some of the explanatory comments? (here and elsewhere where it makes sense?)

================
Comment at: lib/MC/MCFragment.cpp:194
@@ -195,3 +193,3 @@
   assert(BundleSize > 0 &&
          "computeBundlePadding should only be called if bundling is enabled");
   uint64_t BundleMask = BundleSize - 1;
----------------
Can this assert just go away now?

================
Comment at: test/MC/X86/AlignedBundling/nesting.s:36
@@ -37,1 +35,3 @@
   callq bar
+  .bundle_lock
+  callq bar
----------------
Is there some particular reason why the `align_to_end` has been moved to the first bundle_lock directive here? I think we still have the requirement that a nested directive with the `align_to_end` property makes the bundle be aligned to end even if the first directive doesn't have it.


Repository:
  rL LLVM

http://reviews.llvm.org/D19924