Don't create illegal memset with address space >= 256 in memcpyopt.

[mjacob]

Summary

Memcpy / memset for address spaces >= 256 are not supported in the
code generator. This commit adds an extra check that stops the pass from
creating such illegal memsets.

[compnerd]

This seems target dependent. Im not sure why, but, for example, SystemZ does permit it (or at the very least, continues to form the call. Perhaps move this behind a target specific check?

For the test, can you actually run it explicitly against all the architectures we support?

[mjacob]

In D7175#113470, @compnerd wrote:

This seems target dependent. Im not sure why, but, for example, SystemZ does permit it (or at the very least, continues to form the call. Perhaps move this behind a target specific check?

I don't think this is target dependent. See this assert. It fails for any target if the address space is >= 256. As I said on IRC, you probably tested with assertions disabled.

[mjacob]

As Matt Arsenault pointed out on the mailing list, this assert is actually a bug and is implicitly assuming the X86 target. A FIXME was added in rL227191.

I'm abandoning this revision because my assumption, based on that assert, that memset is supported iff the address space is < 256 turned out to be wrong.

lib/Transforms/Scalar/MemCpyOptimizer.cpp

 /// tryMergingIntoMemset - When scanning forward over instructions, we look for
 /// some other patterns to fold away.  In particular, this looks for stores to
 /// neighboring locations of memory.  If it sees enough consecutive ones, it
 /// attempts to merge them together into a memcpy/memset.
 Instruction *MemCpyOpt::tryMergingIntoMemset(Instruction *StartInst,
                                              Value *StartPtr, Value *ByteVal) {
   if (!DL) return nullptr;
 
+  // FIXME: memcpy / memset for address spaces >= 256 are not supported in the
+  // code generator
+  if (cast<PointerType>(StartPtr->getType())->getAddressSpace() >= 256)
+    return nullptr;
+
   // Okay, so we now have a single store that can be splatable.  Scan to find
   // all subsequent stores of the same value to offset from the same pointer.
   // Join these together into ranges, so we can decide whether contiguous blocks
   // are stored.
   MemsetRanges Ranges(*DL);
 
   BasicBlock::iterator BI = StartInst;
   for (++BI; !isa<TerminatorInst>(BI); ++BI) {

test/Transforms/MemCpyOpt/addrspace-256.ll

+; RUN: opt < %s -memcpyopt -S | FileCheck %s
+
+; Memcpy / memset for address spaces >= 256 are not supported in the code
+; generator.  This test makes sure this pass doesn't create new memsets (memcpys
+; are always formed from existing ones).
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+%test = type { i8*, i8*, i8*, i8* }
+
+define void @store_nulls(%test addrspace(256)* %x) {
+  %1 = getelementptr inbounds %test addrspace(256)* %x, i32 0, i32 0
+  store i8* null, i8* addrspace(256)* %1
+  %2 = getelementptr inbounds %test addrspace(256)* %x, i32 0, i32 1
+  store i8* null, i8* addrspace(256)* %2
+  %3 = getelementptr inbounds %test addrspace(256)* %x, i32 0, i32 2
+  store i8* null, i8* addrspace(256)* %3
+  %4 = getelementptr inbounds %test addrspace(256)* %x, i32 0, i32 3
+  store i8* null, i8* addrspace(256)* %4
+; CHECK-LABEL: @store_nulls(
+; CHECK-NOT: memset
+  ret void
+}