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

1488

clang-format

1514

clang-format

[RKSimon]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1595–1600

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

1488

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

1514

See above.

1595–1600

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

1595–1600

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

1585

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

1595

Flip to match the previous if():

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

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

32

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

32

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

591

(#)
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?

601

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)

603

BaseReg + SymScale * IndexReg

614

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

616–624

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

1015

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

1463–1467

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?

1580

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

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

1461

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

591

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

616–624

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

616–624

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

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

591

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

616–624

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

591

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

[coby]

lib/Target/X86/AsmParser/X86AsmParser.cpp

591

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

580

You don't need this line

593

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

1467

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

578–579

Call this NewScale or something more informative than _Scale.

1466

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

1463–1467

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) {
-      PrevState = State;
+    bool checkSetScale(int64_t _Scale, StringRef &ErrMsg) {
+      State = IES_INTEGER;
+      if (checkIndexReg(ErrMsg))
+        return true;
+      IndexReg = TmpReg;
+      Scale = _Scale;
+      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, bool isParsingInlineAsm,
+                          bool isNextMult = false) {
+      IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
+        ErrMsg = "invalid address operation";
+        return true;
+      case IES_MULTIPLY:
+        if (PrevState == IES_REGISTER &&
+            SymRef->getKind() == llvm::MCExpr::Constant) {
+          // IndexRegister + SymbolicScale * Register
+          auto *Constant = cast<MCConstantExpr>(SymRef);
+          if (checkSetScale(Constant->getValue(), ErrMsg))
+            return true;
+        } else {
+          ErrMsg = "invalid scale expression";
+          return true;
+        }
         break;
       case IES_PLUS:
       case IES_MINUS:
       case IES_NOT:
         State = IES_INTEGER;
+        if (isParsingInlineAsm || !isNextMult) {
           Sym = SymRef;
           SymName = SymRefName;
+        }
+        if (SymRef->getKind() == llvm::MCExpr::Constant) {
+          auto *Constant = cast<MCConstantExpr>(SymRef);
+          IC.pushOperand(IC_IMM, Constant->getValue());
+        } else
           IC.pushOperand(IC_IMM);
         break;
       }
+      PrevState = CurrState;
+      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());
 
+  // In MS inline-asm we allow variables to be named as registers, and
+  // give them precedence over actual registers
+  // However - we require the match to be case sensitive
+  if (isParsingInlineAsm() && isParsingIntelSyntax() && RegNo) {
+    StringRef LineBuf(Tok.getIdentifier().data());
+    InlineAsmIdentifierInfo Info;
+    if (SemaCallback->LookupInlineAsmIdentifier(LineBuf, Info, false))
+      return true;
+  }
   // 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
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);
+        bool isNextMult =
+            getLexer().getTok().getKind() == AsmToken::TokenKind::Star;
+        if (SM.onIdentifierExpr(Val, Identifier, ErrMsg, isParsingInlineAsm(),
+                                isNextMult))
+          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, isParsingInlineAsm(),
+                                getLexer().getTok().getKind() ==
+                                    AsmToken::TokenKind::Star))
+          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, isParsingInlineAsm(),
+                                  getLexer().getTok().getKind() ==
+                                      AsmToken::TokenKind::Star))
+            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);
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)
+  int64_t ImmVal = SM.getImm();
+  const MCExpr *Imm = MCConstantExpr::create(ImmVal, getContext());
+  if (Disp) {
+    if (ImmVal && Disp->getKind() == llvm::MCExpr::SymbolRef)
+      // If we have both Disp and Symbol then [Symbol + ImmDisp].
       Disp = MCBinaryExpr::createAdd(Disp, Imm, getContext());
-    else
-      Disp = Imm;  // An immediate displacement only.
+    else if (Disp->getKind() == llvm::MCExpr::Constant && ImmVal) {
+      assert(Disp == SM.getSym() && "It's symbolic disp initialized above");
+      Disp = Imm; // The signed value of the symbolic displacement.
     }
+  } else
+    Disp = Imm; // It can be an immediate displacement only (maybe 0).
 
   // 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

 
 .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

// 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]