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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:646-652
   if (!NumValues.evaluateAsAbsolute(IntNumValues, getAssemblerPtr())) {
+    // Byte fills don't need to know NumValues.
+    if (Size == 1) {
+      emitFill(NumValues, Expr, Loc);
+      return;
+    }
+
----------------
rnk wrote:
> Would it be cleaner to generalize `MCFillFragment` and then we could defer this label difference calculation until after relaxation?
I tend to agree with rnk here. I'm guessing that if there are size concerns about adding fields to MCFillFragment that are not commonly used then it could be worth a new fragment type, potentially derived from MCFillFragment.


================
Comment at: llvm/lib/MC/MCObjectStreamer.cpp:647
   if (!NumValues.evaluateAsAbsolute(IntNumValues, getAssemblerPtr())) {
+    // Byte fills don't need to know NumValues.
+    if (Size == 1) {
----------------
Given that NumValues is passed to emitFill(); did you mean // Byte fills don't need to know IntNumValues?


================
Comment at: llvm/test/MC/X86/pr33586.s:1
+# RUN: llvm-mc -triple i686-unknown-unknown %s -filetype=obj -o /dev/null 
+
----------------
The test looks very similar to the attachment in the PR. Given that the code was from the Linux Kernel we probably ought to generalise and simplify it to avoid any GPL concerns.  


Repository:
  rL LLVM

https://reviews.llvm.org/D46703