[MC] Fold A-B when A's fragment precedes B's fragment

[MaskRay]

Summary

When the MCAssembler is non-null and the MCAsmLayout is null, we can fold A-B
when

• A and B are in the same fragment, or
• A's fragment suceeds B's fragment, and they are not separated by non-data fragments (D69411)

This patch allows folding when A's fragment precedes B's fragment so
that 9997b - . == 0 below can be evaluated as true:

nop
.arch_extension sec
9997:nop
// old behavior: error: expected absolute expression
.if 9997b - . == 0
.endif

Add a case to llvm/test/MC/ARM/directive-if-subtraction.s.
Note: for MCAsmStreamer, we cannot evaluate .if . - 9997b == 0 at parse
time due to MCAsmStreamer::getAssemblerPtr returning nullptr (D45164).

Some Darwin tests check that this folding does not work. Add .p2align 2 to
block some label difference folding or adjust the tests.

[barannikov88]

The logic below is quite difficult to follow. Displacement being negated twice on some code paths is very confusing.
Can it be simplified somehow? Possibly by more intrusive changes?
Otherwise LGTM.

[MaskRay]

In D153096#4427202, @barannikov88 wrote:

The logic below is quite difficult to follow. Displacement being negated twice on some code paths is very confusing.
Can it be simplified somehow? Possibly by more intrusive changes?
Otherwise LGTM.

Thanks for sharing thoughts. I have locally implemented another approach that detects which of FA/FB occurs first, but the code turns out to be more complex, and more complex when the RISC-V linker relaxation code (D153097) is added...
The current code is the best I can think of.

[MaskRay]

rebase

[nickdesaulniers]

llvm/lib/MC/MCExpr.cpp

667–672

is there a more concise way to express this with std::find?

693–698

typo: s/dummay/dummy/

llvm/test/MC/AArch64/arm64-small-data-fixups.s

5

let's keep the indentation of assembler directives consistent

llvm/test/MC/ARM/directive-if-subtraction.s

22

I thought the whole point of this patch is to allow 9997b - . == 0 to fold? Then why is this testing for that to emit an error?

llvm/test/MC/MachO/reloc-diff.s

4

indent

[MaskRay]

improve tests
simplify for (auto FI = FA->getIterator(), FE = SecA.end(); ++FI != FE;) { with a std::find.

[MaskRay]

llvm/test/MC/ARM/directive-if-subtraction.s

22

This test complements the previous test that we still have // ASM:[[@LINE-2]]:5: error: expected absolute expression.

This is related to a larger issue that for now we use MCAsmStreamer::getAssemblerPtr that returns null (related to D45164). I have found that improving this would be challenging.

[nickdesaulniers]

Please fix the last indentation.

llvm/test/MC/ARM/directive-if-subtraction.s

22

I see; the run lines are super perplexing to me; changing the output type from obj (to whatever the implicit default is, another .s file perhaps?) causes the assembler to error? WEIRD

llvm/test/MC/MachO/reloc-diff.s

4

This was marked done but does not look fixed to me, please triple check. Later directives are indented. Please indent this one to be consistent.

[MaskRay]

indent.
Thank you for the review! Adjusting the description slightly...

[MaskRay]

llvm/test/MC/ARM/directive-if-subtraction.s

22

Ah, this patch adds a label difference test that serves as a dual purpose:

• -filetype=obj now folds if 9997b - . == 0
• -filetype=asm cannot fold such expressions

The current summary

Add a case to llvm/test/MC/ARM/directive-if-subtraction.s that we don't evaluate .if . - 9997b == 0 for MCAsmStreamer due to getAssemblerPtr returns nullptr.

llvm/test/MC/MachO/reloc-diff.s

4

Sorry, missed this one. I'll indent it.

(I considered .p2align 2 similar to the function label, as some directives guarding the following body, so I did not indent it. But I can see an argument for just indenting every align directives.

I think this might be a minor issue of the traditional compiler-generated directives. For example, not indenting .globl probably would improve readability.
)

[chapuni]

llvm/lib/MC/MCExpr.cpp

696

Found has become unused in -Asserts.

llvm/lib/MC/MCExpr.cpp

if (Layout) {
     // foo.
     if (SA.isVariable() || SB.isVariable() ||
         FA->getSubsectionNumber() != FB->getSubsectionNumber())
       return;
 
     // Try to find a constant displacement from FA to FB, add the displacement
     // between the offset in FA of SA and the offset in FB of SB.
     int64_t Displacement = SA.getOffset() - SB.getOffset();
+    bool Reverse = false;
+    if (FA == FB) {
+      Reverse = SA.getOffset() < SB.getOffset();
+    } else if (!isa<MCDummyFragment>(FA)) {
+      Reverse = std::find_if(std::next(FA->getIterator()), SecA.end(),
+                             [&](auto &I) { return &I == FB; }) != SecA.end();
+    }
+    if (Reverse) {
+      std::swap(FA, FB);
+      Displacement *= -1;
+    }
+
     bool Found = false;
     for (auto FI = FB->getIterator(), FE = SecA.end(); FI != FE; ++FI) {
       auto DF = dyn_cast<MCDataFragment>(FI);
       if (&*FI == FA) {
         Found = true;
         break;
       }
 
       int64_t Num;
       if (DF) {
         Displacement += DF->getContents().size();
       } else if (auto *FF = dyn_cast<MCFillFragment>(FI);
                  FF && FF->getNumValues().evaluateAsAbsolute(Num)) {
         Displacement += Num * FF->getValueSize();
       } else {
         return;
       }
     }
-    // If FA is found or if FA is a dummy fragment not in the fragment list,
-    // (which means SA is a pending label (see flushPendingLabels)), we can
-    // resolve the difference.
-    if (Found || isa<MCDummyFragment>(FA)) {
-      Addend += Displacement;
-      FinalizeFolding();
-    }
-  }
-}
+    // If the previous loop does not find FA, FA must be a dummy fragment not in
+    // the fragment list (which means SA is a pending label (see
+    // flushPendingLabels)). In either case, we can resolve the difference.
+    assert(Found || isa<MCDummyFragment>(FA));
+    Addend += Reverse ? -Displacement : Displacement;
 
 /// Evaluate the result of an add between (conceptually) two MCValues.
 ///
 /// This routine conceptually attempts to construct an MCValue:
 ///   Result = (Result_A - Result_B + Result_Cst)
 /// from two MCValue's LHS and RHS where
 ///   Result = LHS + RHS
 /// and

llvm/test/MC/AArch64/arm64-small-data-fixups.s

 ; RUN: llvm-mc -triple arm64-apple-darwin -filetype=obj -o - %s | llvm-readobj -r - | FileCheck %s
 
 foo:
   .long 0
+  .p2align 2
 bar:
   .long 1
 
 baz:
   .byte foo - bar
   .short foo - bar
 
 ; CHECK: File: <stdin>

llvm/test/MC/ARM/directive-if-subtraction.s

 // RUN: not llvm-mc -triple armv7a-linux-gnueabihf %s -o /dev/null 2>&1 | FileCheck --check-prefix=ASM %s --implicit-check-not=error:
 // RUN: llvm-mc -triple armv7a-linux-gnueabihf %s -filetype=obj -o %t 2>&1 | count 0
 // RUN: llvm-objdump -d %t | FileCheck --check-prefix=DISASM %s
 
 nop
 // Create a new MCDataFragment due to Subtarget change
 .arch_extension sec
 9997:nop
 .if . - 9997b == 0
 // ASM:    :[[#@LINE-1]]:5: error: expected absolute expression
 // DISASM: orr  r1, r1, #2
 orr r1, r1, #1
 .else
 orr r1, r1, #2
 .endif
 
+nop
+.arch_extension sec
+9997:nop
+.if 9997b - . == 0
+// ASM:    :[[#@LINE-1]]:5: error: expected absolute expression
+// DISASM: orr  r1, r1, #2
+orr r1, r1, #1
+.else
+orr r1, r1, #2
+.endif
 
 @ RUN: not llvm-mc -filetype=obj -triple arm-linux-gnueabihf --defsym=ERR=1 %s -o /dev/null 2>&1 | FileCheck --check-prefix=ARM-ERR %s
 @ RUN: not llvm-mc -filetype=obj -triple thumbv7a-linux-gnueabihf --defsym=ERR=1 %s -o /dev/null 2>&1 | FileCheck --check-prefix=THUMB2-ERR %s
 
 .ifdef ERR
 9997: nop
       .align 4
       nop

llvm/test/MC/MachO/absolutize.s

 // CHECK:     Index: 1
 // CHECK:     Name: __data (5F 5F 64 61 74 61 00 00 00 00 00 00 00 00 00 00)
 // CHECK:     Segment: __DATA (5F 5F 44 41 54 41 00 00 00 00 00 00 00 00 00 00)
 // CHECK:     Address: 0x2B
 // CHECK:     Size: 0x2C
 // CHECK:     Offset: 383
 // CHECK:     Alignment: 0
 // CHECK:     RelocationOffset: 0x1C4
-// CHECK:     RelocationCount: 3
+// CHECK:     RelocationCount: 1
 // CHECK:     Type: Regular (0x0)
 // CHECK:     Attributes [ (0x0)
 // CHECK:     ]
 // CHECK:     Reserved1: 0x0
 // CHECK:     Reserved2: 0x0
 // CHECK:     SectionData (
 // CHECK:       0000: 00000000 00000000 00000000 00000000  |................|
 // CHECK:       0010: FCFFFFFF FCFFFFFF 04000000 04000000  |................|
 // CHECK:       0020: 04000000 04000000 27000000           |........'...|
 // CHECK:     )
 // CHECK:   }
 // CHECK: ]
 // CHECK: Relocations [
 // CHECK:   Section __text {
 // CHECK:     0x27 0 2 n/a GENERIC_RELOC_VANILLA 1 0x0
 // CHECK:     0x9 0 2 n/a GENERIC_RELOC_LOCAL_SECTDIFF 1 0x0
 // CHECK:     0x0 0 2 n/a GENERIC_RELOC_PAIR 1 0x2
 // CHECK:   }
 // CHECK:   Section __data {
 // CHECK:     0x28 0 2 n/a GENERIC_RELOC_VANILLA 1 0x2B
-// CHECK:     0x10 0 2 n/a GENERIC_RELOC_LOCAL_SECTDIFF 1 0x2B
-// CHECK:     0x0 0 2 n/a GENERIC_RELOC_PAIR 1 0x2F
 // CHECK:   }
 // CHECK: ]
 // CHECK: Symbols [
 // CHECK:   Symbol {
 // CHECK:     Name: _text_a (17)
 // CHECK:     Type: Section (0xE)
 // CHECK:     Section: __text (0x1)
 // CHECK:     RefType: UndefinedNonLazy (0x0)

llvm/test/MC/MachO/darwin-x86_64-reloc.s

 // CHECK-NEXT:     Relocation {
 // CHECK-NEXT:       Offset: 0xA5
 // CHECK-NEXT:       PCRel: 0
 // CHECK-NEXT:       Length: 3
 // CHECK-NEXT:       Type: X86_64_RELOC_UNSIGNED (0)
 // CHECK-NEXT:       Symbol: _foo
 // CHECK-NEXT:     }
 // CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x9D
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 3
-// CHECK-NEXT:       Type: X86_64_RELOC_SUBTRACTOR (5)
-// CHECK-NEXT:       Symbol: _prev
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x9D
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 3
-// CHECK-NEXT:       Type: X86_64_RELOC_UNSIGNED (0)
-// CHECK-NEXT:       Symbol: _foo
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
 // CHECK-NEXT:       Offset: 0x95
 // CHECK-NEXT:       PCRel: 0
 // CHECK-NEXT:       Length: 3
 // CHECK-NEXT:       Type: X86_64_RELOC_UNSIGNED (0)
 // CHECK-NEXT:       Symbol: _prev
 // CHECK-NEXT:     }
 // CHECK-NEXT:     Relocation {
 // CHECK-NEXT:       Offset: 0x8D

 // CHECK-NEXT:     Relocation {
 // CHECK-NEXT:       Offset: 0x5C
 // CHECK-NEXT:       PCRel: 1
 // CHECK-NEXT:       Length: 2
 // CHECK-NEXT:       Type: X86_64_RELOC_SIGNED (1)
 // CHECK-NEXT:       Symbol: _prev
 // CHECK-NEXT:     }
 // CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x55
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 2
-// CHECK-NEXT:       Type: X86_64_RELOC_SUBTRACTOR (5)
-// CHECK-NEXT:       Symbol: _bar
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x55
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 2
-// CHECK-NEXT:       Type: X86_64_RELOC_UNSIGNED (0)
-// CHECK-NEXT:       Symbol: _foo
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x4D
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 3
-// CHECK-NEXT:       Type: X86_64_RELOC_SUBTRACTOR (5)
-// CHECK-NEXT:       Symbol: _bar
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x4D
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 3
-// CHECK-NEXT:       Type: X86_64_RELOC_UNSIGNED (0)
-// CHECK-NEXT:       Symbol: _foo
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x45
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 3
-// CHECK-NEXT:       Type: X86_64_RELOC_SUBTRACTOR (5)
-// CHECK-NEXT:       Symbol: _bar
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
-// CHECK-NEXT:       Offset: 0x45
-// CHECK-NEXT:       PCRel: 0
-// CHECK-NEXT:       Length: 3
-// CHECK-NEXT:       Type: X86_64_RELOC_UNSIGNED (0)
-// CHECK-NEXT:       Symbol: _foo
-// CHECK-NEXT:     }
-// CHECK-NEXT:     Relocation {
 // CHECK-NEXT:       Offset: 0x3D
 // CHECK-NEXT:       PCRel: 0
 // CHECK-NEXT:       Length: 3
 // CHECK-NEXT:       Type: X86_64_RELOC_UNSIGNED (0)
 // CHECK-NEXT:       Symbol: _foo
 // CHECK-NEXT:     }
 // CHECK-NEXT:     Relocation {
 // CHECK-NEXT:       Offset: 0x35

llvm/test/MC/MachO/reloc-diff.s

 // RUN: llvm-mc -triple i386-apple-darwin9 %s -filetype=obj -o - | llvm-readobj -r - | FileCheck %s
 
 _local_def:
+        .p2align 2
         .globl _external_def
 _external_def:
 Ltemp:
         ret
 
         .data
         .long _external_def - _local_def
         .long Ltemp - _local_def
 
         .long _local_def - _external_def
         .long Ltemp - _external_def
 
         .long _local_def - Ltemp
         .long _external_def - Ltemp
 
 // CHECK: Relocations [
 // CHECK-NEXT:   Section __data {
 // CHECK-NEXT:     0x10 0 2 n/a GENERIC_RELOC_LOCAL_SECTDIFF 1 0x0
 // CHECK-NEXT:     0x0 0 2 n/a GENERIC_RELOC_PAIR 1 0x0
 // CHECK-NEXT:     0x8 0 2 n/a GENERIC_RELOC_LOCAL_SECTDIFF 1 0x0
 // CHECK-NEXT:     0x0 0 2 n/a GENERIC_RELOC_PAIR 1 0x0
+// CHECK-NEXT:     0x4 0 2 n/a GENERIC_RELOC_LOCAL_SECTDIFF 1 0x0
+// CHECK-NEXT:     0x0 0 2 n/a GENERIC_RELOC_PAIR 1 0x0
+// CHECK-NEXT:     0x0 0 2 n/a GENERIC_RELOC_SECTDIFF 1 0x0
+// CHECK-NEXT:     0x0 0 2 n/a GENERIC_RELOC_PAIR 1 0x0
 // CHECK-NEXT:   }
 // CHECK-NEXT: ]