[PATCH] D124406: [X86] Use indirect addressing for high 2GB of x32 address space

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1696
+    if (Subtarget->isTarget64BitILP32() && !isUInt<31>(Val) &&
+        !AM.hasBaseOrIndexReg())
+      return true;
----------------
hvdijk wrote:
> hvdijk wrote:
> > craig.topper wrote:
> > > hvdijk wrote:
> > > > efriedma wrote:
> > > > > hvdijk wrote:
> > > > > > efriedma wrote:
> > > > > > > The reasoning here seems strange.  For example, suppose I write `void f(int a) { ((char*)0x80000000)[a] = a; }`.  That has a base register, but sign-extension is wrong.
> > > > > > > 
> > > > > > > I guess you're trying to allow negative pointer offsets here, but I think SelectionDAG is throwing away the distinction you need here.  (At the IR level, it's easy to distinguish between the base of a GEP and the offset.)
> > > > > > That does the right thing, that's okay to allow. That results in `movb %dil, -2147483648(%edi)`, and the fact that `%edi` is part of the address means `%edi - 2147483648` is calculated as a 32-bit value, and then zero-extended, so it calculates the exact same thing as `%edi + 2147483648` would if it were possible to do that directly. x86 is weird.
> > > > > Oh, wait, nevermind, I think I see what you mean.  The issue isn't the register; it's the prefix indicating 32-bit addressing, and that only gets emitted if there's a register operand.  
> > > > > 
> > > > > It seems like you should be able to fix the encoding somehow; the prefix has nothing to do with the operands.  Or maybe there's some reason you can't... but in that case, please add a comment explaining.
> > > > Huh, that seems to actually work when I try it, but it's something that GCC doesn't generate, something that GNU objdump disassembles confusingly as something involving %eiz, and something that llvm-objdump disassembles as if there is no address size override. Will work on that, I didn't think that was possible, thanks for the pointer.
> > > I think %eiz mean there is a SIB byte but the index field in the SIB byte is 0b100(no index) and there is a shorter encoding that doesn't use a SIB byte. It's there to disambiguate two encodings that would otherwise print the same string.
> > Trying to get it done, I'm realising this is something that isn't specific to ILP32, this is something we could and should do in LP64 mode as well, except that in LP64 mode we only generate more-complicated-than-necessary code (the same more-complicated-than-necessary code that I am generating for ILP32 here), not wrong code. A change to support this in both ILP32 and LP64 modes will probably end up being larger than I would have liked, but I will give it a go and see if the end result looks manageable enough.
> Unfortunately, although I was able to get something that mostly appeared to work, both for LP64 and for ILP32 mode, by modifying getAddressOperands (X86ISelDAGToDAG.cpp) to set X86::EIZ as an index register just like the GNU disassembler shows it -- which already results in the intended machine code, and also correct textual assembly that we are able to assemble to the intended machine code -- the big problem with it that I was unable to resolve without massively invasive changes was the fact that EIZ is not part of the register classes, meaning that this approach causes MIR verification to fail. I have added a comment hoping to explain that. It seems like something worth pursuing as a later change at some point; for now, I think it is better to stick with this change so that we have a small simple change that results in correct code, even if it is suboptimal.
Instead of using X86::EIZ, could we suppress the sign extending of the immediate and zero extend it instead. Then detect that the immediate isn't an isInt<32> in X86MCCodeEmitter? I guess we'd need to do something for the assembly printer too to print %eiz. So maybe that isn't a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124406