[PATCH] D46703: [MC] Relax .fill size requirements

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the update. I've left some minor comments but I'm overall happy with the change.



================
Comment at: llvm/lib/MC/MCAssembler.cpp:563
     char Data[MaxChunkSize];
-    memcpy(Data, &V, 1);
-    for (unsigned I = 1; I < MaxChunkSize; ++I)
-      Data[I] = Data[0];
-
-    uint64_t Size = FragmentSize;
-    for (unsigned ChunkSize = MaxChunkSize; ChunkSize; ChunkSize /= 2) {
-      StringRef Ref(Data, ChunkSize);
-      for (uint64_t I = 0, E = Size / ChunkSize; I != E; ++I)
-        OW->writeBytes(Ref);
-      Size = Size % ChunkSize;
+    const bool isLittleEndian = Asm.getContext().getAsmInfo()->isLittleEndian();
+    for (unsigned I = 0; I != VSize; ++I) {
----------------
Maybe worth a comment summarising the specification of how .fill forms the value, or hint that the interested reader should look at the GNU as directives manual for a description.  


================
Comment at: llvm/lib/MC/MCAssembler.cpp:568
+    }
+
+    for (unsigned I = VSize; I < MaxChunkSize; ++I)
----------------
I suggest removing the newline here as both loops are constructing Data.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:572
+
+    // Set to largest multiple of VSize in Data.
+    unsigned NumPerChunk = MaxChunkSize / VSize;
----------------
Perhaps // Set ChunkSize to largest multiple of VSize in Data
or do unsigned ChunkSize = MaxChunkSize - MaxChunkSize % VSize
It could also be const unsigned.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:578
+    StringRef Ref(Data, ChunkSize);
+    for (uint64_t I = 0, E = FragmentSize / ChunkSize; I != E; ++I) {
+      OW->writeBytes(Ref);
----------------
I don't think you need the {} in this case.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:581
+    }
+    // do remainder if needed.
+    unsigned TrailingCount = FragmentSize % ChunkSize;
----------------
Could be possible to compress without losing readability, although feel free to ignore as this is just a style choice:
if (unsigned TrailingCount = FragmentSize % ChunkSize)
    OW->writeBytes(StringRef(Data, TrailingCount));



================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:666
 
-  if (IntNumValues < 0) {
-    getContext().getSourceManager()->PrintMessage(
-        Loc, SourceMgr::DK_Warning,
-        "'.fill' directive with negative repeat count has no effect");
-    return;
-  }
+  // Otherwise emit as fragment.
+  MCDataFragment *DF = getOrCreateDataFragment();
----------------
This bit of code is almost identical to emitFill above, the only difference being that Size is used rather than 1. It may be worth passing Size to emitFill above (perhaps with a default value of 1).


================
Comment at: llvm/test/MC/AsmParser/assembler-expressions.s:57
+text3:
+	.fill (text3-text2)/4, 4, 0x12345678
+	nop
----------------
I think we haven't got a big-endian test for .fill when the size is > 1. I think there is a Mips test that uses .fill (hilo-addressing) but it is only .fill 0x30124-8

Probably worth adding one to make sure that the endian reversal code is working.


Repository:
  rL LLVM

https://reviews.llvm.org/D46703