[PATCH] D73227: [ms] [llvm-ml] Use default RIP-relative addressing for x64 MASM.

[Eric Astor via Phabricator via llvm-commits]

epastor marked an inline comment as done.
epastor added inline comments.


================
Comment at: llvm/include/llvm/MC/MCParser/MCAsmParser.h:168
 
-  virtual void setParsingInlineAsm(bool V) = 0;
-  virtual bool isParsingInlineAsm() = 0;
+  virtual void setParsingInlineMSAsm(bool V) = 0;
+  virtual bool isParsingInlineMSAsm() = 0;
----------------
rnk wrote:
> I guess I'd prefer `isParsingMSInlineAsm`. Do you mind uploading that and committing it separately?
Rename split out as a prerequisite differential: https://reviews.llvm.org/D75198


================
Comment at: llvm/include/llvm/MC/MCParser/MCAsmParser.h:171
+
+  virtual bool isParsingMasm() const { return false; }
 
----------------
rnk wrote:
> We already have ASMLexer::LexMasmIntegers. Can this be collapsed with that so that we don't have two indicators of "masm-ness"?
Since that's a Lexer control property, we might need to stick to setting it based on isParsingMasm() instead...


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2062-2063
     return ErrorOperand(Start, ErrMsg);
-  if (isParsingInlineAsm())
+  if (isParsingInlineMSAsm())
     return CreateMemForInlineAsm(RegNo, Disp, BaseReg, IndexReg,
                                  Scale, Start, End, Size, SM.getSymName(),
----------------
rnk wrote:
> Does this not make the check for `isParsingInlindMSAsm()` below redundant? If it's true, we always return here.
Whoops. Yep, revised to set the default base register to RIP in CreateMemForInlineAsm instead.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2070-2072
+    return X86Operand::CreateMem(getPointerWidth(), RegNo, Disp, BaseReg,
+                                 IndexReg, Scale, Start, End, Size,
+                                 /*DefaultBaseReg=*/X86::RIP);
----------------
rnk wrote:
> What ends up going wrong if we set the basereg to RIP in these conditions when it is not already set? I'm wondering if it would be cleaner to do the defaulting here, and then adjust the downstream code to treat RIP-based memory operands the same as no-base memory operands.
I started by doing that, and we ended up with RIP as the BaseReg in many inappropriate situations. The issue is that this only applies when the Operand is matched as a non-absolute reference... which we can't tell at parse time.

It would have been much cleaner, but since the interpretation is context-dependent, I couldn't find a better option. I'm open to suggestions, though!


================
Comment at: llvm/lib/Target/X86/AsmParser/X86Operand.h:63
     unsigned BaseReg;
+    unsigned DefaultBaseReg;
     unsigned IndexReg;
----------------
rnk wrote:
> I'd really like to get by without adding more operand members that don't correspond to parts of the x86 addressing mode.
Agreed. Unfortunately, the only obvious way to do that would be to match the x64 addressing mode more closely, and assume that all non-absolute memory references lacking a BaseReg are RIP-relative... which would break GNU-style assembly, since they made the decision to enforce explicit RIP-relative referencing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73227/new/

https://reviews.llvm.org/D73227