[MemCpyOpt] Be conservative in the face of returns_twice calls

[majnemer]

Summary

It is not safe to forward loads and stores around if calls to setjmp are
present: there might be a call which longjmps causing a memory location
to be initialized with an unexpected value.

This fixes PR27848.

[eli.friedman]

lib/Transforms/Scalar/MemCpyOptimizer.cpp

1381

I don't think all the optimizations in this pass are affected; memset merging seems pretty safe. That said, probably not worth the trouble, given how rare functions which call setjmp are.

[rnk]

I think the right fix is declare that volatile loads and stores "capture" their pointer operands, or to tease out some other function attribute like that. Your reduced test case from IRC shows how we incorrectly redirect volatile stores from y to x: https://ghostbin.com/paste/v5n3f

[majnemer]

Move this change over to functionattrs.

[majnemer]

• Update the LangRef.

[rnk]

lgtm, but let's bug Sanjoy, the person who seems to think the hardest about IPO function attrs right now.

[sanjoy]

I'm fine being bugged, but I think this review is missing some context -- how is "volatile stores escape the location being stored to" related to how we handle setjmp / longjmp?

A explicit test case on why we need this new rule will also be helpful (if not as a test case, then an example in the commit message), since @test_volatile only checks that we implement the rule correctly, but does not explain what the motivation is.

[majnemer]

In D20555#440527, @sanjoy wrote:

I'm fine being bugged, but I think this review is missing some context -- how is "volatile stores escape the location being stored to" related to how we handle setjmp / longjmp?

We substituted the underlying storage for a pointer when calling some function, that function would later perform a volatile store to that pointer. This is important because setjmp and longjmp require any modifications to objects which are loaded after the longjmp to be performed with volatile accesses. This all falls apart if the wrong underlying object is stored to.

A explicit test case on why we need this new rule will also be helpful (if not as a test case, then an example in the commit message), since @test_volatile only checks that we implement the rule correctly, but does not explain what the motivation is.

lib/Transforms/Scalar/MemCpyOptimizer.cpp

bool MemCpyOpt::runOnFunction(Function &F) {
   TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
 
   // If we don't have at least memset and memcpy, there is little point of doing
   // anything here.  These are required by a freestanding implementation, so if
   // even they are disabled, there is no point in trying hard.
   if (!TLI->has(LibFunc::memset) || !TLI->has(LibFunc::memcpy))
     return false;
 
+  // It is not safe to forward loads and stores around if calls to setjmp are
+  // present: there might be a call which longjmps causing a memory location to
+  // be initialized with an unexpected value.
+  if (F.callsFunctionThatReturnsTwice())
+    return false;
+
   while (1) {
     if (!iterateOnFunction(F))
       break;
     MadeChange = true;
   }
 
   MD = nullptr;
   return MadeChange;
 }

test/Transforms/MemCpyOpt/pr27848.ll

+; RUN: opt -S -memcpyopt < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.__jmp_buf_tag = type { [8 x i64], i32, { [16 x i64] } }
+
+; Function Attrs: noinline nounwind uwtable
+declare void @g(i32* nocapture %y, %struct.__jmp_buf_tag* %b) #0
+
+; Function Attrs: noinline nounwind uwtable
+define i32 @f() #0 {
+entry:
+  %x = alloca i32, align 4
+  %y = alloca i32, align 4
+; CHECK:      %[[x:.*]] = alloca i32, align 4
+; CHECK-NEXT: %[[y:.*]] = alloca i32, align 4
+  %b = alloca %struct.__jmp_buf_tag, align 16
+  store volatile i32 1, i32* %x, align 4
+; CHECK:      store volatile i32 1, i32* %[[x]], align 4
+; CHECK-NEXT: call i32 @_setjmp(
+  %call = call i32 @_setjmp(%struct.__jmp_buf_tag* %b) #1
+  %tobool = icmp eq i32 %call, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  %x.0 = load volatile i32, i32* %x, align 4
+; CHECK: %[[x_load_0:.*]] = load volatile i32, i32* %[[x]], align 4
+  br label %return
+
+if.end:                                           ; preds = %entry
+  call void @g(i32* nonnull %y, %struct.__jmp_buf_tag* nonnull %b)
+  %y.load = load i32, i32* %y, align 4
+  store i32 %y.load, i32* %x, align 4
+  %x.1 = load volatile i32, i32* %x, align 4
+; CHECK:      %[[y_load:.*]] = load i32, i32* %[[y]], align 4
+; CHECK-NEXT: store i32 %[[y_load]], i32* %x, align 4
+; CHECK-NEXT: %[[x_load_1:.*]] = load volatile i32, i32* %[[x]], align 4
+  br label %return
+
+return:                                           ; preds = %if.end, %if.then
+  %retval = phi i32 [ %x.0, %if.then ], [ %x.1, %if.end ]
+; CHECK:      %[[retval:.*]] = phi i32 [ %[[x_load_0]], {{.*}} ], [ %[[x_load_1]], {{.*}} ]
+; CHECK-NEXT: ret i32 %[[retval]]
+  ret i32 %retval
+}
+
+; Function Attrs: nounwind returns_twice
+declare i32 @_setjmp(%struct.__jmp_buf_tag*) #1
+
+attributes #0 = { noinline nounwind uwtable }
+attributes #1 = { nounwind returns_twice }