Fix for 9807. Avoid over-relaxation of 8-bit immediates in integer arithmetic instructions.

[DavidKreitzer]

Summary

During some code size experimentation, I observed that ~0.25% of total code size was attributable to this problem. The bug predominantly affects CMP, but I saw a few instances where it affected the other arithmetic ops like ADD, AND, etc.

I opted for this simple localized fix after considering other options that add new infrastructure to be able to tell from the MCFixup itself whether it needs to be relaxed. Note that I removed the mc-x86-disable-arith-relaxation flag, as I think it should no longer be necessary.

[DavidKreitzer]

I decided to go ahead and take care of the FIXME regarding RIP-relative memory references in relaxable arith instructions. I submitted https://llvm.org/bugs/show_bug.cgi?id=23986 and added a new regression test, relax-arith3.s, that illustrates the current problem and verifies that this new (and ultimately simpler) patch fixes it.

The patch passes "make check-all".

[• rafael]

The code change itself LGTM.

Please change the test to use llvm-objdump.

test/MC/ELF/relax-arith2.s

9 ↗

(On Diff #28701)

Can't you used llvm-objdump -d?

[majnemer]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

247 ↗

(On Diff #28701)

Please name this RelaxableOp to match the uppercase convention of the surrounding code.

[DavidKreitzer]

Fixed the style issue pointed out by David and a few others with braces, indentation, and an empty comment line. (Old habits die hard.)

Rafael, I'm happy to change the 2 new tests to use llvm-objdump instead of llvm-readobj if you think that is preferred. I just want to point out that llvm-readobj is used more predominantly in test/MC/ELF:

bash-4.3$ grep llvm-readobj *.s | wc -l
120
bash-4.3$ grep llvm-objdump *.s | wc -l
7

Thanks for the reviews!

[DavidKreitzer]

Modified the new tests to use llvm-objdump instead of llvm-readobj. Thanks for the suggestion, Rafael. They do look nicer this way.

[• rafael]

Thanks, tests LGTM too now!

llvm/trunk/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ELF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MachO.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
-// Option to allow disabling arithmetic relaxation to workaround PR9807, which
-// is useful when running bitwise comparison experiments on Darwin. We should be
-// able to remove this once PR9807 is resolved.
-static cl::opt<bool>
-MCDisableArithRelaxation("mc-x86-disable-arith-relaxation",
-         cl::desc("Disable relaxation of arithmetic instruction for X86"));
-
 static unsigned getFixupKindLog2Size(unsigned Kind) {
   switch (Kind) {
   default:
     llvm_unreachable("invalid fixup kind!");
   case FK_PCRel_1:
   case FK_SecRel_1:
   case FK_Data_1:
     return 0;
static unsigned getRelaxedOpcode(unsigned Op) {
   return getRelaxedOpcodeBranch(Op);
 }
 
 bool X86AsmBackend::mayNeedRelaxation(const MCInst &Inst) const {
   // Branches can always be relaxed.
   if (getRelaxedOpcodeBranch(Inst.getOpcode()) != Inst.getOpcode())
     return true;
 
-  if (MCDisableArithRelaxation)
-    return false;
-
   // Check if this instruction is ever relaxable.
   if (getRelaxedOpcodeArith(Inst.getOpcode()) == Inst.getOpcode())
     return false;
 
 
-  // Check if it has an expression and is not RIP relative.
-  bool hasExp = false;
-  bool hasRIP = false;
-  for (unsigned i = 0; i < Inst.getNumOperands(); ++i) {
-    const MCOperand &Op = Inst.getOperand(i);
-    if (Op.isExpr())
-      hasExp = true;
-
-    if (Op.isReg() && Op.getReg() == X86::RIP)
-      hasRIP = true;
-  }
+  // Check if the relaxable operand has an expression. For the current set of
+  // relaxable instructions, the relaxable operand is always the last operand.
+  unsigned RelaxableOp = Inst.getNumOperands() - 1;
+  if (Inst.getOperand(RelaxableOp).isExpr())
+    return true;
 
-  // FIXME: Why exactly do we need the !hasRIP? Is it just a limitation on
-  // how we do relaxations?
-  return hasExp && !hasRIP;
+  return false;
 }
 
 bool X86AsmBackend::fixupNeedsRelaxation(const MCFixup &Fixup,
                                          uint64_t Value,
                                          const MCRelaxableFragment *DF,
                                          const MCAsmLayout &Layout) const {
   // Relax if the value is too big for a (signed) i8.
   return int64_t(Value) != int64_t(int8_t(Value));

llvm/trunk/test/MC/ELF/relax-arith2.s

+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-objdump -d - | FileCheck  %s
+
+// Test that we avoid relaxing these instructions and instead generate versions
+// that use 8-bit immediate values.
+
+bar:
+// CHECK:      Disassembly of section imul:
+// CHECK-NEXT: imul:
+// CHECK-NEXT:   0: 66 6b db 80                   imulw $-128, %bx, %bx
+// CHECK-NEXT:   4: 66 6b 1c 25 00 00 00 00 7f    imulw $127, 0, %bx
+// CHECK-NEXT:   d: 6b db 00                      imull $0, %ebx, %ebx
+// CHECK-NEXT:  10: 6b 1c 25 00 00 00 00 01       imull $1, 0, %ebx
+// CHECK-NEXT:  18: 48 6b db ff                   imulq $-1, %rbx, %rbx
+// CHECK-NEXT:  1c: 48 6b 1c 25 00 00 00 00 2a    imulq $42, 0, %rbx
+        .section imul,"x"
+        imul $-128, %bx,  %bx
+        imul $127, bar,  %bx
+        imul $0, %ebx, %ebx
+        imul $1, bar,  %ebx
+        imul $-1, %rbx, %rbx
+        imul $42, bar,  %rbx
+
+
+// CHECK:      Disassembly of section and:
+// CHECK-NEXT: and:
+// CHECK-NEXT:   0: 66 83 e3 7f                   andw $127, %bx
+// CHECK-NEXT:   4: 66 83 24 25 00 00 00 00 00    andw $0, 0
+// CHECK-NEXT:   d: 83 e3 01                      andl $1, %ebx
+// CHECK-NEXT:  10: 83 24 25 00 00 00 00 ff       andl $-1, 0
+// CHECK-NEXT:  18: 48 83 e3 2a                   andq $42, %rbx
+// CHECK-NEXT:  1c: 48 83 24 25 00 00 00 00 80    andq $-128, 0
+        .section and,"x"
+        and  $127, %bx
+        andw $0, bar
+        and  $1, %ebx
+        andl $-1, bar
+        and  $42, %rbx
+        andq $-128, bar
+
+// CHECK:      Disassembly of section or:
+// CHECK-NEXT: or:
+// CHECK-NEXT:   0: 66 83 cb 00                   orw $0, %bx
+// CHECK-NEXT:   4: 66 83 0c 25 00 00 00 00 01    orw $1, 0
+// CHECK-NEXT:   d: 83 cb ff                      orl $-1, %ebx
+// CHECK-NEXT:  10: 83 0c 25 00 00 00 00 2a       orl $42, 0
+// CHECK-NEXT:  18: 48 83 cb 80                   orq $-128, %rbx
+// CHECK-NEXT:  1c: 48 83 0c 25 00 00 00 00 7f    orq $127, 0
+        .section or,"x"
+        or  $0, %bx
+        orw $1, bar
+        or  $-1, %ebx
+        orl $42, bar
+        or  $-128, %rbx
+        orq $127, bar
+
+// CHECK:      Disassembly of section xor:
+// CHECK-NEXT: xor:
+// CHECK-NEXT:   0: 66 83 f3 01                   xorw $1, %bx
+// CHECK-NEXT:   4: 66 83 34 25 00 00 00 00 ff    xorw $-1, 0
+// CHECK-NEXT:   d: 83 f3 2a                      xorl $42, %ebx
+// CHECK-NEXT:  10: 83 34 25 00 00 00 00 80       xorl $-128, 0
+// CHECK-NEXT:  18: 48 83 f3 7f                   xorq $127, %rbx
+// CHECK-NEXT:  1c: 48 83 34 25 00 00 00 00 00    xorq $0, 0
+        .section xor,"x"
+        xor  $1, %bx
+        xorw $-1, bar
+        xor  $42, %ebx
+        xorl $-128, bar
+        xor  $127, %rbx
+        xorq $0, bar
+
+// CHECK:      Disassembly of section add:
+// CHECK-NEXT: add:
+// CHECK-NEXT:   0: 66 83 c3 ff                   addw $-1, %bx
+// CHECK-NEXT:   4: 66 83 04 25 00 00 00 00 2a    addw $42, 0
+// CHECK-NEXT:   d: 83 c3 80                      addl $-128, %ebx
+// CHECK-NEXT:  10: 83 04 25 00 00 00 00 7f       addl $127, 0
+// CHECK-NEXT:  18: 48 83 c3 00                   addq $0, %rbx
+// CHECK-NEXT:  1c: 48 83 04 25 00 00 00 00 01    addq $1, 0
+        .section add,"x"
+        add  $-1, %bx
+        addw $42, bar
+        add  $-128, %ebx
+        addl $127, bar
+        add  $0, %rbx
+        addq $1, bar
+
+// CHECK:      Disassembly of section sub:
+// CHECK-NEXT: sub:
+// CHECK-NEXT:   0: 66 83 eb 2a                   subw $42, %bx
+// CHECK-NEXT:   4: 66 83 2c 25 00 00 00 00 80    subw $-128, 0
+// CHECK-NEXT:   d: 83 eb 7f                      subl $127, %ebx
+// CHECK-NEXT:  10: 83 2c 25 00 00 00 00 00       subl $0, 0
+// CHECK-NEXT:  18: 48 83 eb 01                   subq $1, %rbx
+// CHECK-NEXT:  1c: 48 83 2c 25 00 00 00 00 ff    subq $-1, 0
+        .section sub,"x"
+        sub  $42, %bx
+        subw $-128, bar
+        sub  $127, %ebx
+        subl $0, bar
+        sub  $1, %rbx
+        subq $-1, bar
+
+// CHECK:      Disassembly of section cmp:
+// CHECK-NEXT: cmp:
+// CHECK-NEXT:   0: 66 83 fb 80                   cmpw $-128, %bx
+// CHECK-NEXT:   4: 66 83 3c 25 00 00 00 00 7f    cmpw $127, 0
+// CHECK-NEXT:   d: 83 fb 00                      cmpl $0, %ebx
+// CHECK-NEXT:  10: 83 3c 25 00 00 00 00 01       cmpl $1, 0
+// CHECK-NEXT:  18: 48 83 fb ff                   cmpq $-1, %rbx
+// CHECK-NEXT:  1c: 48 83 3c 25 00 00 00 00 2a    cmpq $42, 0
+        .section cmp,"x"
+        cmp  $-128, %bx
+        cmpw $127, bar
+        cmp  $0, %ebx
+        cmpl $1, bar
+        cmp  $-1, %rbx
+        cmpq $42, bar

llvm/trunk/test/MC/ELF/relax-arith3.s

+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-objdump -d - | FileCheck  %s
+
+// Test that we correctly relax these instructions into versions that use
+// 16 or 32 bit immediate values.
+
+bar:
+// CHECK:      Disassembly of section imul:
+// CHECK-NEXT: imul:
+// CHECK-NEXT:   0: 66 69 1d 00 00 00 00 00 00        imulw $0, (%rip), %bx
+// CHECK-NEXT:   9: 69 1d 00 00 00 00 00 00 00 00     imull $0, (%rip), %ebx
+// CHECK-NEXT:  13: 48 69 1d 00 00 00 00 00 00 00 00  imulq $0, (%rip), %rbx
+        .section imul,"x"
+        imul $foo, bar(%rip),  %bx
+        imul $foo, bar(%rip),  %ebx
+        imul $foo, bar(%rip),  %rbx
+
+
+// CHECK:      Disassembly of section and:
+// CHECK-NEXT: and:
+// CHECK-NEXT:   0: 66 81 25 00 00 00 00 00 00        andw $0, (%rip)
+// CHECK-NEXT:   9: 81 25 00 00 00 00 00 00 00 00     andl $0, (%rip)
+// CHECK-NEXT:  13: 48 81 25 00 00 00 00 00 00 00 00  andq $0, (%rip)
+        .section and,"x"
+        andw $foo, bar(%rip)
+        andl $foo, bar(%rip)
+        andq $foo, bar(%rip)
+
+// CHECK:      Disassembly of section or:
+// CHECK-NEXT: or:
+// CHECK-NEXT:   0: 66 81 0d 00 00 00 00 00 00        orw $0, (%rip)
+// CHECK-NEXT:   9: 81 0d 00 00 00 00 00 00 00 00     orl $0, (%rip)
+// CHECK-NEXT:  13: 48 81 0d 00 00 00 00 00 00 00 00  orq $0, (%rip)
+        .section or,"x"
+        orw $foo, bar(%rip)
+        orl $foo, bar(%rip)
+        orq $foo, bar(%rip)
+
+// CHECK:      Disassembly of section xor:
+// CHECK-NEXT: xor:
+// CHECK-NEXT:   0: 66 81 35 00 00 00 00 00 00        xorw $0, (%rip)
+// CHECK-NEXT:   9: 81 35 00 00 00 00 00 00 00 00     xorl $0, (%rip)
+// CHECK-NEXT:  13: 48 81 35 00 00 00 00 00 00 00 00  xorq $0, (%rip)
+        .section xor,"x"
+        xorw $foo, bar(%rip)
+        xorl $foo, bar(%rip)
+        xorq $foo, bar(%rip)
+
+// CHECK:      Disassembly of section add:
+// CHECK-NEXT: add:
+// CHECK-NEXT:   0: 66 81 05 00 00 00 00 00 00        addw $0, (%rip)
+// CHECK-NEXT:   9: 81 05 00 00 00 00 00 00 00 00     addl $0, (%rip)
+// CHECK-NEXT:  13: 48 81 05 00 00 00 00 00 00 00 00  addq $0, (%rip)
+        .section add,"x"
+        addw $foo, bar(%rip)
+        addl $foo, bar(%rip)
+        addq $foo, bar(%rip)
+
+// CHECK:      Disassembly of section sub:
+// CHECK-NEXT: sub:
+// CHECK-NEXT:   0: 66 81 2d 00 00 00 00 00 00        subw $0, (%rip)
+// CHECK-NEXT:   9: 81 2d 00 00 00 00 00 00 00 00     subl $0, (%rip)
+// CHECK-NEXT:  13: 48 81 2d 00 00 00 00 00 00 00 00  subq $0, (%rip)
+        .section sub,"x"
+        subw $foo, bar(%rip)
+        subl $foo, bar(%rip)
+        subq $foo, bar(%rip)
+
+// CHECK:      Disassembly of section cmp:
+// CHECK-NEXT: cmp:
+// CHECK-NEXT:   0: 66 81 3d 00 00 00 00 00 00        cmpw $0, (%rip)
+// CHECK-NEXT:   9: 81 3d 00 00 00 00 00 00 00 00     cmpl $0, (%rip)
+// CHECK-NEXT:  13: 48 81 3d 00 00 00 00 00 00 00 00  cmpq $0, (%rip)
+        .section cmp,"x"
+        cmpw $foo, bar(%rip)
+        cmpl $foo, bar(%rip)
+        cmpq $foo, bar(%rip)