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

Can't you used llvm-objdump -d?

[majnemer]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

247

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!

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));

test/MC/ELF/relax-arith2.s

+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-readobj -s -sd | FileCheck  %s
+
+// Test that we avoid relaxing these instructions and instead generate versions
+// that use 8-bit immediate values.
+
+bar:
+// CHECK:      Name: imul
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 666BDB80 666B1C25 00000000 7F6BDB00
+// CHECK-NEXT:   0010: 6B1C2500 00000001 486BDBFF 486B1C25
+// CHECK-NEXT:   0020: 00000000 2A
+// CHECK-NEXT: )
+        .section imul
+        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:      Name: and
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 6683E37F 66832425 00000000 0083E301
+// CHECK-NEXT:   0010: 83242500 000000FF 4883E32A 48832425
+// CHECK-NEXT:   0020: 00000000 80
+// CHECK-NEXT: )
+        .section and
+        and  $127, %bx
+        andw $0, bar
+        and  $1, %ebx
+        andl $-1, bar
+        and  $42, %rbx
+        andq $-128, bar
+
+// CHECK:      Name: or
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 6683CB00 66830C25 00000000 0183CBFF
+// CHECK-NEXT:   0010: 830C2500 0000002A 4883CB80 48830C25
+// CHECK-NEXT:   0020: 00000000 7F
+// CHECK-NEXT: )
+        .section or
+        or  $0, %bx
+        orw $1, bar
+        or  $-1, %ebx
+        orl $42, bar
+        or  $-128, %rbx
+        orq $127, bar
+
+// CHECK:      Name: xor
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 6683F301 66833425 00000000 FF83F32A
+// CHECK-NEXT:   0010: 83342500 00000080 4883F37F 48833425
+// CHECK-NEXT:   0020: 00000000 00
+// CHECK-NEXT: )
+        .section xor
+        xor  $1, %bx
+        xorw $-1, bar
+        xor  $42, %ebx
+        xorl $-128, bar
+        xor  $127, %rbx
+        xorq $0, bar
+
+// CHECK:      Name: add
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 6683C3FF 66830425 00000000 2A83C380
+// CHECK-NEXT:   0010: 83042500 0000007F 4883C300 48830425
+// CHECK-NEXT:   0020: 00000000 01
+// CHECK-NEXT: )
+        .section add
+        add  $-1, %bx
+        addw $42, bar
+        add  $-128, %ebx
+        addl $127, bar
+        add  $0, %rbx
+        addq $1, bar
+
+// CHECK:      Name: sub
+// CHECK:      SectionData (
+// CHECK-NEXT:   000: 6683EB2A 66832C25 00000000 8083EB7F
+// CHECK-NEXT:   010: 832C2500 00000000 4883EB01 48832C25
+// CHECK-NEXT:   020: 00000000 FF
+// CHECK-NEXT: )
+        .section sub
+        sub  $42, %bx
+        subw $-128, bar
+        sub  $127, %ebx
+        subl $0, bar
+        sub  $1, %rbx
+        subq $-1, bar
+
+// CHECK:      Name: cmp
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 6683FB80 66833C25 00000000 7F83FB00
+// CHECK-NEXT:   0010: 833C2500 00000001 4883FBFF 48833C25
+// CHECK-NEXT:   0020: 00000000 2A
+// CHECK-NEXT: )
+        .section cmp
+        cmp  $-128, %bx
+        cmpw $127, bar
+        cmp  $0, %ebx
+        cmpl $1, bar
+        cmp  $-1, %rbx
+        cmpq $42, bar

test/MC/ELF/relax-arith3.s

+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-readobj -s -sd | FileCheck  %s
+
+// Test that we correctly relax these instructions into versions that use
+// 16 or 32 bit immediate values.
+
+bar:
+// CHECK:      Name: imul
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 66691D00 00000000 00691D00 00000000
+// CHECK-NEXT:   0010: 00000048 691D0000 00000000 0000
+// CHECK-NEXT: )
+        .section imul
+        imul $foo, bar(%rip),  %bx
+        imul $foo, bar(%rip),  %ebx
+        imul $foo, bar(%rip),  %rbx
+
+
+// CHECK:      Name: and
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 66812500 00000000 00812500 00000000
+// CHECK-NEXT:   0010: 00000048 81250000 00000000 0000
+// CHECK-NEXT: )
+        .section and
+        andw $foo, bar(%rip)
+        andl $foo, bar(%rip)
+        andq $foo, bar(%rip)
+
+// CHECK:      Name: or
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 66810D00 00000000 00810D00 00000000
+// CHECK-NEXT:   0010: 00000048 810D0000 00000000 0000
+// CHECK-NEXT: )
+        .section or
+        orw $foo, bar(%rip)
+        orl $foo, bar(%rip)
+        orq $foo, bar(%rip)
+
+// CHECK:      Name: xor
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 66813500 00000000 00813500 00000000
+// CHECK-NEXT:   0010: 00000048 81350000 00000000 0000
+// CHECK-NEXT: )
+        .section xor
+        xorw $foo, bar(%rip)
+        xorl $foo, bar(%rip)
+        xorq $foo, bar(%rip)
+
+// CHECK:      Name: add
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 66810500 00000000 00810500 00000000
+// CHECK-NEXT:   0010: 00000048 81050000 00000000 0000
+// CHECK-NEXT: )
+        .section add
+        addw $foo, bar(%rip)
+        addl $foo, bar(%rip)
+        addq $foo, bar(%rip)
+
+// CHECK:      Name: sub
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 66812D00 00000000 00812D00 00000000
+// CHECK-NEXT:   0010: 00000048 812D0000 00000000 0000
+// CHECK-NEXT: )
+        .section sub
+        subw $foo, bar(%rip)
+        subl $foo, bar(%rip)
+        subq $foo, bar(%rip)
+
+// CHECK:      Name: cmp
+// CHECK:      SectionData (
+// CHECK-NEXT:   0000: 66813D00 00000000 00813D00 00000000
+// CHECK-NEXT:   0010: 00000048 813D0000 00000000 0000
+// CHECK-NEXT: )
+        .section cmp
+        cmpw $foo, bar(%rip)
+        cmpl $foo, bar(%rip)
+        cmpq $foo, bar(%rip)