X86 Asm can't work properly with symbolic Scale

[avt77]

Summary

The current Asm does not support symbolic Scale at all. It's only possible to use immediate as Scale. This patch is the first one to close PR33848

[avt77]

I updated 2 tests in trunk to demonstarte the difference in code generation after this patch aplying. Now we can clearly see the issues and how they could be resolved with help of this patch.

[avt77]

I re-based the the code after the patch about replacing of assertions with corresponding diagnostic error messages. As result changes in tests show that X86 Asm supports symbolic Scale/Disp properly now (including its negative value).

[avt77]

Hi Reviewers?
Do you have any questions about this patch?

[RKSimon]

lib/Target/X86/AsmParser/X86AsmParser.cpp

571–572

(typo) ErrMsg = "Invalid operand of multiplication";

1461

clang-format

1485

clang-format

[RKSimon]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1564

What is this accomplishing? Pulling on ImmVal makes sense, but the rest looks wrong - for instance, if Disp is a MCExpr::Constant doesn't that constant value is overriden by Imm instead of being added together?

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1461

I'm using clang-format for every new patch prepared for phab and this style was generated by clang-format (I've just checked it again).

1485

See above.

1564

Oh, yes: if Disp is Constant and Imm != 0 then we should add 2 values and it was already done in one of the previous versions but disappeared. Tnank you. I'll add this clause asap. But the current override of Disp with Imm is correct as well because we have Disp == SM.getSym(). On the other hand the condition "if (!Disp || (Disp->getKind() == llvm::MCExpr::Constant && ImmVal))" looks wrong.

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1564

Oh and Oh: I recalled the situation:

if (Disp is Constant) it means Disp == SM.getSym() (see assertion): it's the same value that's why we don't replace one with other. I could simplify it like here:

else if (!Disp)
  Disp = Imm; // It can be an immediate displacement only (maybe 0).

OK?
One more time: if (SM.getSym() != nullptr) and it's a constant then SM.getImm() returns the value of the constant.

[avt77]

Hope, my comments now are OK.

[RKSimon]

A few minors - does anyone have any more comments?

lib/Target/X86/AsmParser/X86AsmParser.cpp

1554

Another use of SM.getImm() - is it safe to move SM.getImm() earlier to replace this as well?

1564

Flip to match the previous if():

else if (ImmVal && Disp->getKind() == llvm::MCExpr::Constant) {

test/MC/X86/intel-syntax-3.s

9–10

Now that these are passing - move them to another of the intel-syntax test files and add suitable checks

[avt77]

test/MC/X86/intel-syntax-3.s

9–10

In fact I kept it here specialy to show the difference but OK: I'll do it.

[avt77]

The last requirements from Simon were fixed.

[coby]

A few minors - does anyone have any more comments?

Will have a look by tomorrow (at most)

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

599

if comment (#) evaluate to false (i.e. MSVC inline-assembler does not support Assembly Constants, and we do not plan to do it either) than you should error here appropriately (i.e. "symbolic scale aren't supported on inline-asm)

601

BaseReg + SymScale * IndexReg

601

What about other operations?
If we are to deal with a symbolic constant as a constant than it should be able to participate in whatever operation an integer is allowed

604

(#)
Note that the two conditions (isParsingInlineAsm) and (SymRef->getKind() == llvm::MCExpr::Constant) never coexist in the current code due to the way Assembly Constants are being treated when parsing MS InlineAsm.
I'm not sure whether it is intended or not - does MSVC inline assembler allow any kind of Assembly Constants?

605

Note that this patch breaks 'ms-inline-asm.c' on 'clang/test/Sema'

995

Rebase
This is a part of an old code of mine which got reverted

1434–1440

I suggest we'll adopt a more general approach and deal with assembly constants as a whole, and not just this private case. wouldn't it simplify the work?

1549

if (Disp = SM.getSym() && isParsingInlineAsm())

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1432

Suggested simplification (instead of the other changes as well):

} else if (!isParsingInlineAsm()) {
  if (getParser().parsePrimaryExpr(Val, End))
    return Error(Tok.getLoc(), "Unexpected identifier!");
  if (Val->getKind() == MCExpr::Constant) {
    int64_t TmpInt;
    Val->evaluateAsAbsolute(TmpInt);
    StringRef ErrMsg;
    if (SM.onInteger(TmpInt, ErrMsg))
      return Error(Tok.getLoc(), ErrMsg);
  } else
    SM.onIdentifierExpr(Val, Identifier);

Will allow the use of an assembly constant as an immediate, as integral part of whatever compound expression (scale / other).

[coby]

adding Reid.

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

604

Do you mean that this function can't be called if isParsingInlineAsm == true?

605

I'm no sure I understand you here. As result of the patch we have the following:

void t3() {

• __asm { mov eax, [eax] UndeclaredId } // expected-error {{unknown token in expression}} expected-error {{use of undeclared label 'UndeclaredId'}}

+ __asm { mov eax, [eax] UndeclaredId } // expected-error {{invalid address operation}} expected-error {{use of undeclared label 'UndeclaredId'}}

// FIXME: Only emit one diagnostic here.
// expected-error@+3 {{use of undeclared label 'A'}}
// expected-error@+2 {{unexpected type name 'A': expected expression}}

• // expected-error@+1 {{unknown token in expression}}

+ // expected-error@+1 {{invalid address operation}}

__asm { mov eax, [eax] A }

}

As you see "unknown token" message was replaced by "invalid address operation". Does it mean "break" for this test?

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

605

Don't know the reason but "bullet" above means "minus".

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

604

I mean that (isParsingInlineAsm() && SymRef->getKind() == llvm::MCExpr::Constant) will never yield 'true' on current code state.
IMHO, we should aim for assembly level only.
So, combining the two we may limit this patch to Assembly parsing only

605

As long as the test remains unchanged :)
But again, I don't think that MS InlineAsm should be affected by this change.

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

604

In this case I could simply produce error message if (isParsingInlineAsm() && SymRef->getKind() == llvm::MCExpr::Constant) , right?

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

604

Currently, yes. But I frankly believe we should completely ignore InlineAsm (i.e. confine the patch to assembly level only, as proposed on line 1461), or conduct a better research regarding the way assembly constants are represented (likely - aren't) by msvc inline-assembler, and act upon the findings

[avt77]

I applied all changes suggested by Coby.

[coby]

In D35621#838847, @avt77 wrote:

I applied all changes suggested by Coby.

I'm sorry, I guess I haven't been clear: I meant that the suggested code blob at line 1449 is a practical replacement of the entire patch (apart from the added diagnostics).
It seems to pass your added tests, and not to break anything else, Can you verify it answers your cause(s)?

[avt77]

I removed all changes (apart from the added diagnostics). Now it should fit to Coby requirements. Right?

[coby]

few minors + add fixes in clang tests to adopt the added diagnostics.
nothing else has caught my eye

lib/Target/X86/AsmParser/X86AsmParser.cpp

593

You don't need this line

606

CurrState is redundant, you can use 'PrevState = State' as in the original code.

1438

line is redundant (ErrMsg is defined on the encapsulating function's scope)

[avt77]

Last minor fixes done.

[avt77]

The Clang changes added.

[avt77]

I restored the patch with LLVM part only.

[avt77]

I created a ne review to show changes in Clang: D36735

[rnk]

lib/Target/X86/AsmParser/X86AsmParser.cpp

579–591

Call this NewScale or something more informative than _Scale.

1437

evaluateAsAbsolute may fail. I think you should rewrite this like so:

if (const auto *CE = dyn_cast<MCConstantExpr>(Val)) {
  if (SM.onInteger(CE->getValue(), ErrMsg)
    return Error(Tok.getLoc(), ErrMsg);
}

That's all evaluateAsAbsolute is doing for you behind the scenes anyway.

[avt77]

Aplied Reid's requirements.

[avt77]

Coby, Reid, is eveything OK? Could you get LGTM?

[coby]

In D35621#845117, @avt77 wrote:

Coby, Reid, is eveything OK? Could you get LGTM?

added one minor.
+ I'm currently having a patch on review (D36793) which contains most of the intended goals of this one (everything but the new error, afaik), can you verify it answers your needs?

lib/Target/X86/AsmParser/X86AsmParser.cpp

1434

this line is redundant, its semantics contained in the check performed on the next one.

[avt77]

+ I'm currently having a patch on review (D36793) which contains most of the intended goals of this one (everything but the new error, afaik), can you verify it answers your needs?

Yes, I see that everything I need here is included in D36793. When are you going to commit it? BTW, I have some questions about D36793 but I'll include them in that review.

Finally, as I understand I should abandom this review because now it's a part of D36793 (including tests, etc.), right?

[coby]

In D35621#845237, @avt77 wrote:

+ I'm currently having a patch on review (D36793) which contains most of the intended goals of this one (everything but the new error, afaik), can you verify it answers your needs?

Yes, I see that everything I need here is included in D36793. When are you going to commit it?

as soon as it'll get approved..

Finally, as I understand I should abandom this review because now it's a part of D36793 (including tests, etc.), right?

indeed. note that your added diagnostics are not part of D36793, so consider adding them afterwards if you still fancy it.

[avt77]

All changes from this review (excluding error diagnostic) were included in D36793.

lib/Target/X86/AsmParser/X86AsmParser.cpp

void onRShift() {
       case IES_RPAREN:
       case IES_REGISTER:
         State = IES_RSHIFT;
         IC.pushOperator(IC_RSHIFT);
         break;
       }
       PrevState = CurrState;
     }
+    bool checkIndexReg(StringRef &ErrMsg) {
+      if (IndexReg) {
+        ErrMsg = "BaseReg/IndexReg already set!";
+        return true;
+      }
+      return false;
+    }
     bool onPlus(StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_INTEGER:
       case IES_RPAREN:
       case IES_REGISTER:
         State = IES_PLUS;
         IC.pushOperator(IC_PLUS);
         if (CurrState == IES_REGISTER && PrevState != IES_MULTIPLY) {
           // If we already have a BaseReg, then assume this is the IndexReg with
           // a scale of 1.
           if (!BaseReg) {
             BaseReg = TmpReg;
           } else {
-            if (IndexReg) {
-              ErrMsg = "BaseReg/IndexReg already set!";
+            if (checkIndexReg(ErrMsg))
               return true;
-            }
             IndexReg = TmpReg;
             Scale = 1;
           }
         }
         break;
       }
       PrevState = CurrState;
       return false;
bool onMinus(StringRef &ErrMsg) {
         } else
           IC.pushOperator(IC_NEG);
         if (CurrState == IES_REGISTER && PrevState != IES_MULTIPLY) {
           // If we already have a BaseReg, then assume this is the IndexReg with
           // a scale of 1.
           if (!BaseReg) {
             BaseReg = TmpReg;
           } else {
-            if (IndexReg) {
-              ErrMsg = "BaseReg/IndexReg already set!";
+            if (checkIndexReg(ErrMsg))
               return true;
-            }
             IndexReg = TmpReg;
             Scale = 1;
           }
         }
         break;
       }
       PrevState = CurrState;
       return false;
void onNot() {
       PrevState = CurrState;
     }
 
     bool onRegister(unsigned Reg, StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
-        break;
+        ErrMsg = "Invalid operation with register";
+        return true;
       case IES_PLUS:
       case IES_LPAREN:
         State = IES_REGISTER;
         TmpReg = Reg;
         IC.pushOperand(IC_REGISTER);
         break;
       case IES_MULTIPLY:
         // Index Register - Scale * Register
         if (PrevState == IES_INTEGER) {
-          if (IndexReg) {
-            ErrMsg = "BaseReg/IndexReg already set!";
+          if (checkIndexReg(ErrMsg))
             return true;
-          }
           State = IES_REGISTER;
           IndexReg = Reg;
           // Get the scale and replace the 'Scale * Register' with '0'.
           Scale = IC.popOperand();
           if (checkScale(Scale, ErrMsg))
             return true;
           IC.pushOperand(IC_IMM);
           IC.popOperator();
         } else {
-          State = IES_ERROR;
+          ErrMsg = "Invalid operand of multiplication";
+          return true;
         }
         break;
       }
       PrevState = CurrState;
       return false;
     }
-    void onIdentifierExpr(const MCExpr *SymRef, StringRef SymRefName) {
+    bool checkSetScale(int64_t NewScale, StringRef &ErrMsg) {
+      if (checkIndexReg(ErrMsg))
+        return true;
+      IndexReg = TmpReg;
+      Scale = NewScale;
+      if (checkScale(Scale, ErrMsg))
+        return true;
+      // Get the scale and replace the 'Register * Scale' with '0'.
+      IC.popOperator();
+      return false;
+    }
+    bool onIdentifierExpr(const MCExpr *SymRef, StringRef SymRefName,
+                          StringRef &ErrMsg) {
       PrevState = State;
       switch (State) {
       default:
         State = IES_ERROR;
+        ErrMsg = "invalid address operation";
+        return true;
         break;
       case IES_PLUS:
       case IES_MINUS:
       case IES_NOT:
         State = IES_INTEGER;
         Sym = SymRef;
         SymName = SymRefName;
         IC.pushOperand(IC_IMM);
         break;
       }
+      return false;
     }
     bool onInteger(int64_t TmpInt, StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
         break;
       case IES_PLUS:
       case IES_MINUS:
       case IES_NOT:
       case IES_OR:
       case IES_XOR:
       case IES_AND:
       case IES_LSHIFT:
       case IES_RSHIFT:
       case IES_DIVIDE:
       case IES_MOD:
       case IES_MULTIPLY:
       case IES_LPAREN:
         State = IES_INTEGER;
         if (PrevState == IES_REGISTER && CurrState == IES_MULTIPLY) {
-          // Index Register - Register * Scale
-          if (IndexReg) {
-            ErrMsg = "BaseReg/IndexReg already set!";
+          if (checkSetScale(TmpInt, ErrMsg))
             return true;
-          }
-          IndexReg = TmpReg;
-          Scale = TmpInt;
-          if (checkScale(Scale, ErrMsg))
-            return true;
-          // Get the scale and replace the 'Register * Scale' with '0'.
-          IC.popOperator();
         } else {
           IC.pushOperand(IC_IMM, TmpInt);
         }
         break;
       }
       PrevState = CurrState;
       return false;
     }
bool X86AsmParser::ParseRegister(unsigned &RegNo,
   }
 
   RegNo = MatchRegisterName(Tok.getString());
 
   // If the match failed, try the register name as lowercase.
   if (RegNo == 0)
     RegNo = MatchRegisterName(Tok.getString().lower());
 
   // The "flags" register cannot be referenced directly.
   // Treat it as an identifier instead.
   if (isParsingInlineAsm() && isParsingIntelSyntax() && RegNo == X86::EFLAGS)
     RegNo = 0;
 
   if (!is64BitMode()) {
     // FIXME: This should be done using Requires<Not64BitMode> and
     // Requires<In64BitMode> so "eiz" usage in 64-bit instructions can be also
     // checked.
case AsmToken::Identifier: {
       StringRef Identifier = Tok.getString();
       UpdateLocLex = false;
       if (TK != AsmToken::String && !ParseRegister(TmpReg, IdentLoc, End)) {
         if (SM.onRegister(TmpReg, ErrMsg))
           return Error(Tok.getLoc(), ErrMsg);
       } else if (ParseIntelNamedOperator(Identifier, SM)) {
         UpdateLocLex = true;
       } else if (!isParsingInlineAsm()) {
         if (getParser().parsePrimaryExpr(Val, End))
           return Error(Tok.getLoc(), "Unexpected identifier!");
-        SM.onIdentifierExpr(Val, Identifier);
+        if (Val->getKind() == MCExpr::Constant) {
+          if (const auto *CE = dyn_cast<MCConstantExpr>(Val)) {
+            if (SM.onInteger(CE->getValue(), ErrMsg))
+              return Error(Tok.getLoc(), ErrMsg);
+          }
+        } else if (SM.onIdentifierExpr(Val, Identifier, ErrMsg))
+          return Error(IdentLoc, ErrMsg);
       } else if (unsigned OpKind = IdentifyIntelOperator(Identifier)) {
         if (OpKind == IOK_OFFSET) 
           return Error(IdentLoc, "Dealing OFFSET operator as part of"
             "a compound immediate expression is yet to be supported");
         int64_t Val = ParseIntelOperator(OpKind);
         if (!Val)
           return true;
         if (SM.onInteger(Val, ErrMsg))
           return Error(IdentLoc, ErrMsg);
       } else if (Identifier.find('.') != StringRef::npos &&
             PrevTK == AsmToken::RBrac) {
           return false;
       } else {
         InlineAsmIdentifierInfo &Info = SM.getIdentifierInfo();
         if (ParseIntelIdentifier(Val, Identifier, Info,
                                  /*Unevaluated=*/false, End))
           return true;
-        SM.onIdentifierExpr(Val, Identifier);
+        if (SM.onIdentifierExpr(Val, Identifier, ErrMsg))
+          return Error(IdentLoc, ErrMsg);
       }
       break;
     }
     case AsmToken::Integer: {
       if (isParsingInlineAsm() && SM.getAddImmPrefix())
         InstInfo->AsmRewrites->emplace_back(AOK_ImmPrefix, Tok.getLoc());
       // Look for 'b' or 'f' following an Integer as a directional label
       SMLoc Loc = getTok().getLoc();
       int64_t IntVal = getTok().getIntVal();
       End = consumeToken();
       UpdateLocLex = false;
       if (getLexer().getKind() == AsmToken::Identifier) {
         StringRef IDVal = getTok().getString();
         if (IDVal == "f" || IDVal == "b") {
           MCSymbol *Sym =
               getContext().getDirectionalLocalSymbol(IntVal, IDVal == "b");
           MCSymbolRefExpr::VariantKind Variant = MCSymbolRefExpr::VK_None;
           const MCExpr *Val =
               MCSymbolRefExpr::create(Sym, Variant, getContext());
           if (IDVal == "b" && Sym->isUndefined())
             return Error(Loc, "invalid reference to undefined symbol");
           StringRef Identifier = Sym->getName();
-          SM.onIdentifierExpr(Val, Identifier);
+          if (SM.onIdentifierExpr(Val, Identifier, ErrMsg))
+            return Error(Loc, ErrMsg);
           End = consumeToken();
         } else {
           if (SM.onInteger(IntVal, ErrMsg))
             return Error(Loc, ErrMsg);
         }
       } else {
         if (SM.onInteger(IntVal, ErrMsg))
           return Error(Loc, ErrMsg);
       }
       break;
X86AsmParser::ParseIntelBracExpression(unsigned SegReg, SMLoc Start,
   // Parse [ Symbol + ImmDisp ] and [ BaseReg + Scale*IndexReg + ImmDisp ].  We
   // may have already parsed an immediate displacement before the bracketed
   // expression.
   IntelExprStateMachine SM(ImmDisp, /*StopOnLBrac=*/false, /*AddImmPrefix=*/true);
   if (ParseIntelExpression(SM, End))
     return nullptr;
 
   const MCExpr *Disp = nullptr;
   if (const MCExpr *Sym = SM.getSym()) {
     // A symbolic displacement.
     Disp = Sym;
     if (isParsingInlineAsm())
       RewriteIntelBracExpression(*InstInfo->AsmRewrites, SM.getSymName(),
                                  ImmDisp, SM.getImm(), BracLoc, StartInBrac,
                                  End);
   }
 
   if (SM.getImm() || !Disp) {
     const MCExpr *Imm = MCConstantExpr::create(SM.getImm(), getContext());
     if (Disp)
       Disp = MCBinaryExpr::createAdd(Disp, Imm, getContext());
     else
       Disp = Imm;  // An immediate displacement only.
   }
 
   // Parse struct field access.  Intel requires a dot, but MSVC doesn't.  MSVC
   // will in fact do global lookup the field name inside all global typedefs,
   // but we don't emulate that.
   if ((Parser.getTok().getKind() == AsmToken::Identifier ||
        Parser.getTok().getKind() == AsmToken::Dot ||
        Parser.getTok().getKind() == AsmToken::Real) &&
       Parser.getTok().getString().find('.') != StringRef::npos) {

test/MC/X86/intel-syntax-3.s

 // RUN: not llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s 2> %t.err
 // RUN: FileCheck --check-prefix=CHECK-STDERR < %t.err %s
 
-_test:
-// CHECK-LABEL: _test:
-// CHECK: xorl    %eax, %eax
-
-  xor EAX, EAX
-  ret
-
-.set  number, 8
 .global _foo
 
 .text
   .global main
 main:
 
-// CHECK-STDERR:  error: unknown token in expression
-  lea RDX, [RAX * number + RBX + _foo]
-
-// CHECK-STDERR:  error: unknown token in expression
-  lea RDX, [_foo + RAX * number + RBX]
-
-// CHECK-STDERR:  error: unknown token in expression
-  lea RDX, [number + RAX * number + RCX]
-
-// CHECK-STDERR:  error: unknown token in expression
-  lea RDX, [_foo + RAX * number]
-
-// CHECK-STDERR:  error: unknown token in expression
-  lea RDX, [_foo + RAX * number + RBX]
-
-// CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
-  lea RDX, [number * RAX + RBX + _foo]
-
-// CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
-  lea RDX, [_foo + number * RAX + RBX]
-
-// CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
-  lea RDX, [8 + number * RAX + RCX]
-
 // CHECK-STDERR: scale factor in address must be 1, 2, 4 or 8
 lea RDX, [unknown_number * RAX + RBX + _foo]
 
 // CHECK-STDERR: error: BaseReg/IndexReg already set!
 lea RDX, [4 * RAX + 27 * RBX + _pat]

test/MC/X86/intel-syntax.s

 // RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel %s > %t 2> %t.err
 // RUN: FileCheck < %t %s
 // RUN: FileCheck --check-prefix=CHECK-STDERR < %t.err %s
 
 _test:
   xor EAX, EAX
   ret
 
 .set  number, 8
 .global _foo
 
 .text
   .global main
 main:
 
 // CHECK: leaq    _foo(%rbx,%rax,8), %rdx
+  lea RDX, [RAX * number + RBX + _foo]
+
+// CHECK: leaq    _foo(%rbx,%rax,8), %rdx
+  lea RDX, [_foo + RAX * number + RBX]
+
+// CHECK: leaq    8(%rcx,%rax,8), %rdx
+  lea RDX, [number + RAX * number + RCX]
+
+// CHECK: leaq    _foo(,%rax,8), %rdx
+  lea RDX, [_foo + RAX * number]
+
+// CHECK: leaq    _foo(%rbx,%rax,8), %rdx
+  lea RDX, [_foo + RAX * number + RBX]
+
+// CHECK: leaq    _foo(%rbx,%rax,8), %rdx
+  lea RDX, [number * RAX + RBX + _foo]
+
+// CHECK: leaq    _foo(%rbx,%rax,8), %rdx
+  lea RDX, [_foo + number * RAX + RBX]
+
+// CHECK: leaq    8(%rcx,%rax,8), %rdx
+  lea RDX, [8 + number * RAX + RCX]
+
+// CHECK: leaq    _foo(%rbx,%rax,8), %rdx
   lea RDX, [8 * RAX + RBX      + _foo]
 
 // CHECK: leaq _foo(%rbx,%rax,8), %rdx
   lea RDX, [_foo + 8 * RAX + RBX]
 
 // CHECK: leaq 8(%rcx,%rax,8), %rdx
   lea RDX, [8 + RAX * 8 + RCX]
 
 // CHECK: leaq 8(%rcx,%rax,8), %rdx
   lea RDX, [number + 8 * RAX + RCX]
 
 // CHECK: leaq _foo(,%rax,8), %rdx
   lea RDX, [_foo + RAX * 8]
 
 // CHECK:  leaq _foo(%rbx,%rax,8), %rdx
   lea RDX, [_foo + RAX * 8 + RBX]
 
-// CHECK: leaq 8(%rax), %rdx
+// CHECK: leaq    -8(%rax), %rdx
   lea RDX, [RAX - number]
 // CHECK: leaq -8(%rax), %rdx
   lea RDX, [RAX - 8]
 
 // CHECK: leaq    _foo(%rax), %rdx
   lea RDX, [RAX + _foo]
 // CHECK: leaq    8(%rax), %rdx
   lea RDX, [RAX + number]