fix memcpy/memset/memmove lowering when optimizing for size

[spatel]

Summary

Fixing MinSize attribute handling was discussed in D11363. This is a prerequisite patch to doing that.

The handling of OptSize when lowering mem* functions was broken on Darwin because it wants to ignore -Os for these cases, but the existing logic also made it ignore -Oz (MinSize).

The Linux change demonstrates a widespread problem. The backend doesn't recognize the MinSize attribute by itself; it assumes that MinSize implies OptSize. Fixing this more generally will be a follow-on patch or two.

[mkuper]

Thanks for taking care of this, Sanjay.
It would be really nice to be consistent.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4156 ↗

(On Diff #30837)

Should start with a capital, I think.

4157 ↗

(On Diff #30837)

Same.

4159 ↗

(On Diff #30837)

Is this Darwin/non-Darwin distinction documented anywhere?
And do we respect it anywhere else, aside from the memfunc issue?

[spatel]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

4156 ↗

(On Diff #30837)

Yep, will fix.

4159 ↗

(On Diff #30837)

AFAIK, it's not documented anywhere publicly. It goes back to the PowerPC days at Apple (so a bit strange that it's not also in the PPC lowering).

At the time, Apple built almost all internal projects with -Os and made that the default opt level for developers too (not sure if this is still true today).

The mem* lowering was an exception to the general rule to always optimize for size because it was clearly a win on some benchmark to inline those. So I assume this little hack was added to gcc and made it's way to LLVM too. :)

The existing test case references: rdar://8821501 Perhaps someone at Apple can give us an update. It may be that we don't even need to make this distinction for Darwin anymore?

Ie, on x86 an inline 'rep movs' might be a better answer, and the AArch64 backend does not currently make any concessions for size on any platform:

MaxStoresPerMemset = MaxStoresPerMemsetOptSize = 8;
MaxStoresPerMemcpy = MaxStoresPerMemcpyOptSize = 4;
MaxStoresPerMemmove = MaxStoresPerMemmoveOptSize = 4;

[spatel]

Patch updated:
Fixed capitalization of variables.

[mkuper]

Well, the code LGTM.

As to whether this is still the desired behavior - your call.
I'm ok with committing this as essentially NFC, and then ripping it out later if it turns out to be no longer necessary.

[spatel]

In D11568#214102, @mkuper wrote:

Well, the code LGTM.

As to whether this is still the desired behavior - your call.
I'm ok with committing this as essentially NFC, and then ripping it out later if it turns out to be no longer necessary.

Thanks, Michael.
Yes, my intention with this patch was to leave Darwin codegen as-is except to fix the bug of not producing smaller code at -Oz. Anything more should be a separate patch. Adding Jim Grosbach to see if we can get any clarification on that.

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

while (Size != 0) {
 
     MemOps.push_back(VT);
     Size -= VTSize;
   }
 
   return true;
 }
 
+static bool shouldLowerMemFuncForSize(const MachineFunction &MF) {
+  const Function *F = MF.getFunction();
+  bool HasMinSize = F->hasFnAttribute(Attribute::MinSize);
+  bool HasOptSize = F->hasFnAttribute(Attribute::OptimizeForSize);
+
+  // On Darwin, -Os means optimize for size without hurting performance, so
+  // only really optimize for size when -Oz (MinSize) is used.
+  if (MF.getTarget().getTargetTriple().isOSDarwin())
+    return HasMinSize;
+  return HasOptSize || HasMinSize;
+}
+
 static SDValue getMemcpyLoadsAndStores(SelectionDAG &DAG, SDLoc dl,
                                        SDValue Chain, SDValue Dst,
                                        SDValue Src, uint64_t Size,
                                        unsigned Align, bool isVol,
                                        bool AlwaysInline,
                                        MachinePointerInfo DstPtrInfo,
                                        MachinePointerInfo SrcPtrInfo) {
   // Turn a memcpy of undef to nop.
   if (Src.getOpcode() == ISD::UNDEF)
     return Chain;
 
   // Expand memcpy to a series of load and store ops if the size operand falls
   // below a certain threshold.
   // TODO: In the AlwaysInline case, if the size is big then generate a loop
   // rather than maybe a humongous number of loads and stores.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   std::vector<EVT> MemOps;
   bool DstAlignCanChange = false;
   MachineFunction &MF = DAG.getMachineFunction();
   MachineFrameInfo *MFI = MF.getFrameInfo();
-  bool OptSize = MF.getFunction()->hasFnAttribute(Attribute::OptimizeForSize);
+  bool OptSize = shouldLowerMemFuncForSize(MF);
   FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Dst);
   if (FI && !MFI->isFixedObjectIndex(FI->getIndex()))
     DstAlignCanChange = true;
   unsigned SrcAlign = DAG.InferPtrAlignment(Src);
   if (Align > SrcAlign)
     SrcAlign = Align;
   StringRef Str;
   bool CopyFromStr = isMemSrcFromString(Src, Str);
static SDValue getMemmoveLoadsAndStores(SelectionDAG &DAG, SDLoc dl,
 
   // Expand memmove to a series of load and store ops if the size operand falls
   // below a certain threshold.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   std::vector<EVT> MemOps;
   bool DstAlignCanChange = false;
   MachineFunction &MF = DAG.getMachineFunction();
   MachineFrameInfo *MFI = MF.getFrameInfo();
-  bool OptSize = MF.getFunction()->hasFnAttribute(Attribute::OptimizeForSize);
+  bool OptSize = shouldLowerMemFuncForSize(MF);
   FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Dst);
   if (FI && !MFI->isFixedObjectIndex(FI->getIndex()))
     DstAlignCanChange = true;
   unsigned SrcAlign = DAG.InferPtrAlignment(Src);
   if (Align > SrcAlign)
     SrcAlign = Align;
   unsigned Limit = AlwaysInline ? ~0U : TLI.getMaxStoresPerMemmove(OptSize);
 
static SDValue getMemsetStores(SelectionDAG &DAG, SDLoc dl,
 
   // Expand memset to a series of load/store ops if the size operand
   // falls below a certain threshold.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   std::vector<EVT> MemOps;
   bool DstAlignCanChange = false;
   MachineFunction &MF = DAG.getMachineFunction();
   MachineFrameInfo *MFI = MF.getFrameInfo();
-  bool OptSize = MF.getFunction()->hasFnAttribute(Attribute::OptimizeForSize);
+  bool OptSize = shouldLowerMemFuncForSize(MF);
   FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Dst);
   if (FI && !MFI->isFixedObjectIndex(FI->getIndex()))
     DstAlignCanChange = true;
   bool IsZeroVal =
     isa<ConstantSDNode>(Src) && cast<ConstantSDNode>(Src)->isNullValue();
   if (!FindOptimalMemOpLowering(MemOps, TLI.getMaxStoresPerMemset(OptSize),
                                 Size, (DstAlignCanChange ? 0 : Align), 0,
                                 true, IsZeroVal, false, true, DAG, TLI))

llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM,
   if (Subtarget->useSoftFloat() || Subtarget->isThumb1Only() ||
       !Subtarget->hasVFP2())
     setSchedulingPreference(Sched::RegPressure);
   else
     setSchedulingPreference(Sched::Hybrid);
 
   //// temporary - rewrite interface to use type
   MaxStoresPerMemset = 8;
-  MaxStoresPerMemsetOptSize = Subtarget->isTargetDarwin() ? 8 : 4;
+  MaxStoresPerMemsetOptSize = 4;
   MaxStoresPerMemcpy = 4; // For @llvm.memcpy -> sequence of stores
-  MaxStoresPerMemcpyOptSize = Subtarget->isTargetDarwin() ? 4 : 2;
+  MaxStoresPerMemcpyOptSize = 2;
   MaxStoresPerMemmove = 4; // For @llvm.memmove -> sequence of stores
-  MaxStoresPerMemmoveOptSize = Subtarget->isTargetDarwin() ? 4 : 2;
+  MaxStoresPerMemmoveOptSize = 2;
 
   // On ARM arguments smaller than 4 bytes are extended, so all arguments
   // are at least 4 bytes aligned.
   setMinStackArgumentAlignment(4);
 
   // Prefer likely predicted branches to selects on out-of-order cores.
   PredictableSelectIsExpensive = Subtarget->isLikeA9();
 

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

   setTargetDAGCombine(ISD::SETCC);
   setTargetDAGCombine(ISD::INTRINSIC_WO_CHAIN);
   setTargetDAGCombine(ISD::BUILD_VECTOR);
   setTargetDAGCombine(ISD::MUL);
   setTargetDAGCombine(ISD::XOR);
 
   computeRegisterProperties(Subtarget->getRegisterInfo());
 
-  // On Darwin, -Os means optimize for size without hurting performance,
-  // do not reduce the limit.
   MaxStoresPerMemset = 16; // For @llvm.memset -> sequence of stores
-  MaxStoresPerMemsetOptSize = Subtarget->isTargetDarwin() ? 16 : 8;
+  MaxStoresPerMemsetOptSize = 8;
   MaxStoresPerMemcpy = 8; // For @llvm.memcpy -> sequence of stores
-  MaxStoresPerMemcpyOptSize = Subtarget->isTargetDarwin() ? 8 : 4;
+  MaxStoresPerMemcpyOptSize = 4;
   MaxStoresPerMemmove = 8; // For @llvm.memmove -> sequence of stores
-  MaxStoresPerMemmoveOptSize = Subtarget->isTargetDarwin() ? 8 : 4;
+  MaxStoresPerMemmoveOptSize = 4;
   setPrefLoopAlignment(4); // 2^4 bytes.
 
   // Predictable cmov don't hurt on atom because it's in-order.
   PredictableSelectIsExpensive = !Subtarget->isAtom();
   EnableExtLdPromotion = true;
   setPrefFunctionAlignment(4); // 2^4 bytes.
 
   verifyIntrinsicTables();

llvm/trunk/test/CodeGen/X86/memcpy.ll

 ; DARWIN: movq
 ; DARWIN: movq
 ; DARWIN: movq
 ; DARWIN: movq
 ; DARWIN: movq
 ; DARWIN: movq
 }
 
-; FIXME: Both Linux and Darwin should lower to a memcpy call; minsize is on.
 define void @test3_minsize(i8* nocapture %A, i8* nocapture %B) nounwind minsize noredzone {
   tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* %A, i8* %B, i64 64, i32 1, i1 false)
   ret void
 ; LINUX-LABEL: test3_minsize:
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
-; LINUX: movq
+; LINUX: memcpy
 
 ; DARWIN-LABEL: test3_minsize:
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
+; DARWIN: memcpy
 }
 
-; FIXME: Darwin should lower to a memcpy call; minsize is on.
 define void @test3_minsize_optsize(i8* nocapture %A, i8* nocapture %B) nounwind optsize minsize noredzone {
   tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* %A, i8* %B, i64 64, i32 1, i1 false)
   ret void
 ; LINUX-LABEL: test3_minsize_optsize:
 ; LINUX: memcpy
 
 ; DARWIN-LABEL: test3_minsize_optsize:
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
-; DARWIN: movq
+; DARWIN: memcpy
 }
 
 ; Large constant memcpy's should be inlined when not optimizing for size.
 define void @test4(i8* nocapture %A, i8* nocapture %B) nounwind noredzone {
 entry:
   tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* %A, i8* %B, i64 64, i32 1, i1 false)
   ret void
 ; LINUX-LABEL: test4: