Extend or truncate __ptr32/__ptr64 pointers when dereferenced.

[akhuang]

Summary

A while ago I implemented the functionality to lower Microsoft ptr32
and ptr64 pointers, which are basically stored as 32-bit and 64-bit
pointers and are extended/truncated to the appropriate pointer size when
dereferenced. Apparently I never implemented this extend/truncate when
dereferencing part, so that is what this patch attempts to do.

For 32-bit pointers in 64-bit builds I put the sign/zero extend in
matchAddress in X86ISelDAGToDAG, but loading from 64-bit pointers in
32-bit builds fails in the legalizer before that point, which is why
this code is in two different places. Not sure if this is a good way of doing
this.

Bug: https://bugs.llvm.org/show_bug.cgi?id=42359

[hans]

which is why this code is in two different places

Could both cases be handled in the place that runs before the legalizer?

Hopefully Craig can take a look too, he's the expert here.

llvm/test/CodeGen/X86/mixed-ptr-sizes.ll

132

This just shows the sign extension. I would add a check line for the actual load too, to make sure that's correct and also to make the test easier to understand. Same for the tests below.

[JohnReagan]

Thanks for tackling this. Again, comparing this to what I have on my proprietary system with dual sized pointers, I have some observations (and some confusions on my part I'm sure)

For 32-bit targets, can you even talk about ptr64? My old 32-bit platforms don't allow it at all. However, your custom lowering for 32-bit targets seems to imply that you want to support ptr64 containers on 32-bit platforms by always truncating them. Or is the model that a 32-bit target would simply not use those two address spaces and therefore not generate the size/zero extension?

I'm not sure what "custom" means but you only set that for 32-bit targets. Does that mean the zero/sign-extension on the load doesn't occur for 64-bit targets? You still want to zero/sign-extend a ptr32 in a 64-bit target, yes? From what I think I read, the truncation of ptr32 for the store will only be for on 32-bit targets.

My memory was that you'd be able to mix-and-match ptr32 and ptr64 on 64-bit targets since you still need ptr32 to deal with legacy OS interfaces and such.

[hans]

In D81517#2084827, @JohnReagan wrote:

Thanks for tackling this. Again, comparing this to what I have on my proprietary system with dual sized pointers, I have some observations (and some confusions on my part I'm sure)

For 32-bit targets, can you even talk about ptr64? My old 32-bit platforms don't allow it at all. However, your custom lowering for 32-bit targets seems to imply that you want to support ptr64 containers on 32-bit platforms by always truncating them. Or is the model that a 32-bit target would simply not use those two address spaces and therefore not generate the size/zero extension?

It's following MSVC, whose behaviour can be observed here: https://godbolt.org/z/uNwjMW

[craig.topper]

I'm worried that load/store aren't the only things that can use these pointers. Or that we will have transformed the load/store into a target specific load/store before type legalizaton kicks in. I'm not an expert on address spaces, but it feels to me like we should have an address space cast in IR before the pointers are used by load/store. And that the address space cast is what should be turned into trunc or extend. @arsenm what do you think?

[arsenm]

In D81517#2085442, @craig.topper wrote:

I'm worried that load/store aren't the only things that can use these pointers. Or that we will have transformed the load/store into a target specific load/store before type legalizaton kicks in. I'm not an expert on address spaces, but it feels to me like we should have an address space cast in IR before the pointers are used by load/store. And that the address space cast is what should be turned into trunc or extend. @arsenm what do you think?

I don't think the addressing mode matching should be responsible for inserting this (it's also suspicious that this would need to look at the memory address space)

[craig.topper]

Part of me thinks we should just rewrite the loads/stores as early as possible using combineLoad/combineStore to use a ADDRSPACECAST. Then let the existing legalization of ADDRSPACECAST take care of it. And not change the setOperationAction for loads/store. I get a little nervous that something somewhere will care if an i32 load is Custom instead of Legal.

[akhuang]

Put addr space casting in combineLoad/combineStore.

[akhuang]

Thanks for the comments! I moved the addr space stuff to combineLoad/combineStore, which ends up being a lot simpler.

[craig.topper]

Can you rebase this patch? I added a 32-bit version of mixed-ptr-sizes.ll as mixed-ptr-sizes-i686.ll the other day. And the X86ISelDAGToDAG.cpp changes should already be done in trunk as well.

[akhuang]

Rebase and add more check lines to the tests.

[RKSimon]

@akhuang If you rebase the tests can now be regenerated with the update script

[akhuang]

@RKSimon oh, cool. thanks!

[akhuang]

Rebase tests.

[RKSimon]

please can you check the patch and rebase again? It looks like some of the code is now in the wrong function?

[akhuang]

Rebase, move code to the correct place

[akhuang]

sorry about that! It now should be the intended change.

[craig.topper]

LGTM

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

if (Parent &&
       // that are not a MemSDNode, and thus don't have proper addrspace info.
       Parent->getOpcode() != ISD::INTRINSIC_W_CHAIN && // unaligned loads, fixme
       Parent->getOpcode() != ISD::INTRINSIC_VOID && // nontemporal stores
       Parent->getOpcode() != X86ISD::TLSCALL && // Fixme
       Parent->getOpcode() != X86ISD::ENQCMD && // Fixme
       Parent->getOpcode() != X86ISD::ENQCMDS && // Fixme
       Parent->getOpcode() != X86ISD::EH_SJLJ_SETJMP && // setjmp
       Parent->getOpcode() != X86ISD::EH_SJLJ_LONGJMP) { // longjmp
+
     unsigned AddrSpace =
       cast<MemSDNode>(Parent)->getPointerInfo().getAddrSpace();
-    // AddrSpace 256 -> GS, 257 -> FS, 258 -> SS.
-    if (AddrSpace == 256)
+    if (AddrSpace == X86AS::GS)
       AM.Segment = CurDAG->getRegister(X86::GS, MVT::i16);
-    if (AddrSpace == 257)
+    if (AddrSpace == X86AS::FS)
       AM.Segment = CurDAG->getRegister(X86::FS, MVT::i16);
-    if (AddrSpace == 258)
+    if (AddrSpace == X86AS::SS)
       AM.Segment = CurDAG->getRegister(X86::SS, MVT::i16);
   }
 
   // Save the DL and VT before calling matchAddress, it can invalidate N.
   SDLoc DL(N);
   MVT VT = N.getSimpleValueType();
 
   if (matchAddress(N, AM))

llvm/lib/Target/X86/X86ISelLowering.cpp

                                     Ld->getPointerInfo(),
                                     Ld->getOriginalAlign(),
                                     Ld->getMemOperand()->getFlags());
       SDValue BoolVec = DAG.getBitcast(RegVT, IntLoad);
       return DCI.CombineTo(N, BoolVec, IntLoad.getValue(1), true);
     }
   }
 
+  // Cast ptr32 and ptr64 pointers to the default address space before a load.
+  unsigned AddrSpace = Ld->getAddressSpace();
+  if (AddrSpace == X86AS::PTR64 || AddrSpace == X86AS::PTR32_SPTR ||
+      AddrSpace == X86AS::PTR32_UPTR) {
+    MVT PtrVT = TLI.getPointerTy(DAG.getDataLayout());
+    if (PtrVT != Ld->getBasePtr().getSimpleValueType()) {
+      SDValue Cast =
+          DAG.getAddrSpaceCast(dl, PtrVT, Ld->getBasePtr(), AddrSpace, 0);
+      return DAG.getLoad(RegVT, dl, Ld->getChain(), Cast, Ld->getPointerInfo(),
+                         Ld->getOriginalAlign(),
+                         Ld->getMemOperand()->getFlags());
+    }
+  }
+
   return SDValue();
 }
 
 /// If V is a build vector of boolean constants and exactly one of those
 /// constants is true, return the operand index of that true element.
 /// Otherwise, return -1.
 static int getOneTrueElt(SDValue V) {
   // This needs to be a build vector of booleans.

         return EmitTruncSStore(false /* Unsigned saturation */, St->getChain(),
                                dl, Val, St->getBasePtr(),
                                St->getMemoryVT(), St->getMemOperand(), DAG);
     }
 
     return SDValue();
   }
 
+  // Cast ptr32 and ptr64 pointers to the default address space before a store.
+  unsigned AddrSpace = St->getAddressSpace();
+  if (AddrSpace == X86AS::PTR64 || AddrSpace == X86AS::PTR32_SPTR ||
+      AddrSpace == X86AS::PTR32_UPTR) {
+    MVT PtrVT = TLI.getPointerTy(DAG.getDataLayout());
+    if (PtrVT != St->getBasePtr().getSimpleValueType()) {
+      SDValue Cast =
+          DAG.getAddrSpaceCast(dl, PtrVT, St->getBasePtr(), AddrSpace, 0);
+      return DAG.getStore(St->getChain(), dl, StoredVal, Cast,
+                          St->getPointerInfo(), St->getOriginalAlign(),
+                          St->getMemOperand()->getFlags(), St->getAAInfo());
+    }
+  }
+
   // Turn load->store of MMX types into GPR load/stores.  This avoids clobbering
   // the FP state in cases where an emms may be missing.
   // A preferable solution to the general problem is to figure out the right
   // places to insert EMMS.  This qualifies as a quick hack.
 
   // Similarly, turn load->store of i64 into double load/stores in 32-bit mode.
   if (VT.getSizeInBits() != 64)
     return SDValue();

llvm/test/CodeGen/X86/mixed-ptr-sizes.ll

-; RUN: llc < %s | FileCheck --check-prefixes=CHECK %s
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck --check-prefixes=CHECK,X64 %s
+; RUN: llc -mtriple=i686-windows-msvc < %s | FileCheck --check-prefixes=X86 %s
 ; RUN: llc -O0 < %s | FileCheck --check-prefixes=CHECK %s
 
 ; Source to regenerate:
 ; struct Foo {
 ;   int * __ptr32 p32;
 ;   int * __ptr64 p64;
 ;   __attribute__((address_space(9))) int *p_other;
 ; };

 ; CHECK:       movl %edx, %eax
 entry:
   %0 = addrspacecast i32 addrspace(271)* %i to i32 addrspace(9)*
   %p32 = getelementptr inbounds %struct.Foo, %struct.Foo* %f, i64 0, i32 2
   store i32 addrspace(9)* %0, i32 addrspace(9)** %p32, align 8
   tail call void @use_foo(%struct.Foo* %f)
   ret void
 }
+
+; X64-LABEL: test_load_sptr32:
+; X64: movslq  %ecx, %rax
+; X64: movl (%rax), %eax
+define i32 @test_load_sptr32(i32 addrspace(270)* %i) {
+entry:
+  %0 = load i32, i32 addrspace(270)* %i, align 4
+  ret i32 %0
+}
+
+; X64-LABEL: test_load_uptr32:
+; X64: movl %ecx, %eax
+; X64: movl (%rax), %eax
+define i32 @test_load_uptr32(i32 addrspace(271)* %i) {
+entry:
+  %0 = load i32, i32 addrspace(271)* %i, align 4
+  ret i32 %0
+}
+
+; X86-LABEL: _test_load_ptr64:
+; X86: movl 4(%esp), %eax
+; X86: movl (%eax), %eax
+define i32 @test_load_ptr64(i32 addrspace(272)* %i) {
+entry:
+  %0 = load i32, i32 addrspace(272)* %i, align 8
+  ret i32 %0
+}
+
+; X64-LABEL: test_store_sptr32:
+; X64: movslq %ecx, %rax
+; X64: movl %edx, (%rax)
+define void @test_store_sptr32(i32 addrspace(270)* %s, i32 %i) {
+entry:
+  store i32 %i, i32 addrspace(270)* %s, align 4
+  ret void
+}
+
+; X64-LABEL: test_store_uptr32:
+; X64: movl %ecx, %eax
+; X64: movl %edx, (%rax)
+define void @test_store_uptr32(i32 addrspace(271)* %s, i32 %i) {
+entry:
+  store i32 %i, i32 addrspace(271)* %s, align 4
+  ret void
+}
+
+; X86-LABEL: _test_store_ptr64:
+; X86: movl 12(%esp), %eax
+; X86: movl 4(%esp), %ecx
+; X86: movl %eax, (%ecx)
+define void @test_store_ptr64(i32 addrspace(272)* %s, i32 %i) {
+entry:
+  store i32 %i, i32 addrspace(272)* %s, align 8
+  ret void
+}