From 011fb5bf8b31316472fccb1a19c91912246df9b2 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Sat, 28 Mar 2020 11:03:14 -0700 Subject: [PATCH] [CodeGen] Fix sinking local values in lpads with phis There was already a test case for landingpads to handle this case, but I had forgotten to consider PHI instructions preceding the EH_LABEL in the landingpad. PR45261 --- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp | 17 +++++++++- llvm/test/CodeGen/X86/sink-local-value.ll | 36 ++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp index 5ac3606dc662..2638b1e8a05c 100644 --- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp +++ llvm/lib/CodeGen/SelectionDAG/FastISel.cpp @@ -225,6 +225,21 @@ static bool isRegUsedByPhiNodes(unsigned DefReg, return false; } +static bool isTerminatingEHLabel(MachineBasicBlock *MBB, MachineInstr &MI) { + // Ignore non-EH labels. + if (!MI.isEHLabel()) + return false; + + // Any EH label outside a landing pad must be for an invoke. Consider it a + // terminator. + if (!MBB->isEHPad()) + return true; + + // If this is a landingpad, the first non-phi instruction will be an EH_LABEL. + // Don't consider that label to be a terminator. + return MI.getIterator() != MBB->getFirstNonPHI(); +} + /// Build a map of instruction orders. Return the first terminator and its /// order. Consider EH_LABEL instructions to be terminators as well, since local /// values for phis after invokes must be materialized before the call. @@ -233,7 +248,7 @@ void FastISel::InstOrderMap::initialize( unsigned Order = 0; for (MachineInstr &I : *MBB) { if (!FirstTerminator && - (I.isTerminator() || (I.isEHLabel() && &I != &MBB->front()))) { + (I.isTerminator() || isTerminatingEHLabel(MBB, I))) { FirstTerminator = &I; FirstTerminatorOrder = Order; } diff --git a/llvm/test/CodeGen/X86/sink-local-value.ll b/llvm/test/CodeGen/X86/sink-local-value.ll index b0e511ac1189..f7d861ac9b6c 100644 --- llvm/test/CodeGen/X86/sink-local-value.ll +++ llvm/test/CodeGen/X86/sink-local-value.ll @@ -145,6 +145,42 @@ try.cont: ; preds = %entry, %lpad ; CHECK: retl +define i32 @lpad_phi() personality i32 (...)* @__gxx_personality_v0 { +entry: + store i32 42, i32* @sink_across + invoke void @may_throw() + to label %try.cont unwind label %lpad + +lpad: ; preds = %entry + %p = phi i32 [ 11, %entry ] ; Trivial, but -O0 keeps it + %0 = landingpad { i8*, i32 } + catch i8* null + store i32 %p, i32* @sink_across + br label %try.cont + +try.cont: ; preds = %entry, %lpad + %r.0 = phi i32 [ 13, %entry ], [ 55, %lpad ] + ret i32 %r.0 +} + +; The constant materialization should be *after* the stores to sink_across, but +; before any EH_LABEL. + +; CHECK-LABEL: lpad_phi: +; CHECK: movl $42, sink_across +; CHECK: movl $13, %{{[a-z]*}} +; CHECK: .Ltmp{{.*}}: +; CHECK: calll may_throw +; CHECK: .Ltmp{{.*}}: +; CHECK: jmp .LBB{{.*}} +; CHECK: .LBB{{.*}}: # %lpad +; CHECK-NEXT: .Ltmp{{.*}}: +; CHECK: movl {{.*}}, sink_across +; CHECK: movl $55, %{{[a-z]*}} +; CHECK: .LBB{{.*}}: # %try.cont +; CHECK: retl + + ; Function Attrs: nounwind readnone speculatable declare void @llvm.dbg.value(metadata, metadata, metadata) #0 uctors are now run concurrently, via cooperative scheduling (Guile Fibers). The Jami service previously relied on blocking sleeps while polling for D-Bus services to become ready after forking a process; this wouldn't work anymore since while blocking the service process wouldn't be given the chance to finish starting. The new reliance on Fibers in Shepherd's fork+exec-command in the helper 'send-dbus' procedure also meant that it wouldn't work outside of Shepherd anymore. Finally, the 'start-service' Shepherd procedure used in the test suite would cause the Jami daemon to be spawned multiple times (a bug introduced in Shepherd 0.9.0). To fix/simplify these problems, this change does the following: 1. Use the Guile AC/D-Bus library for D-Bus communication, which simplify things, such as avoiding the need to fork 'dbus-send' processes. 2. The non-blocking 'sleep' version of Fiber is used for the 'with-retries' waiting syntax. 3. A 'dbus' package variant is used to adjust the session bus configuration, tailoring it for the use case at hand. 4. Avoid start-service in the tests, preferring 'jami-service-available?' for now. * gnu/build/jami-service.scm (parse-dbus-reply, strip-quotes) (deserialize-item, serialize-boolean, dbus-dict->alist) (dbus-array->list, parse-account-ids, parse-account-details) (parse-contacts): Delete procedures. (%send-dbus-binary, %send-dbus-bus, %send-dbus-user, %send-dbus-group) (%send-dbus-debug): Delete parameters. (jami-service-running?): New procedure. (send-dbus/configuration-manager): Rename to... (call-configuration-manager-method): ... this. Turn METHOD into a positional argument. Turn ARGUMENTS into an optional argument. Invoke `call-dbus-method' instead of `send-dbus', adjusting callers accordingly. (get-account-ids, id->account-details, id->account-details) (id->volatile-account-details, username->id, add-account remove-account) (username->contacts, remove-contact, add-contact, set-account-details) (set-all-moderators, username->all-moderators?, username->moderators) (set-moderator): Adjust accordingly. (with-retries, send-dbus, dbus-available-services) (dbus-service-available?): Move to ... * gnu/build/dbus-service.scm: ... this new module. (send-dbus): Rewrite to use the Guile AC/D-Bus library. (%dbus-query-timeout, sleep*): New variables. (%current-dbus-connection): New parameter. (initialize-dbus-connection!, argument->signature-type) (call-dbus-method): New procedures. (dbus-available-services): Adjust accordingly. * gnu/local.mk (GNU_SYSTEM_MODULES): Register new module. * gnu/packages/glib.scm (dbus-for-jami): New variable. * gnu/services/telephony.scm: (jami-configuration)[dbus]: Default to dbus-for-jami. (jami-dbus-session-activation): Write a D-Bus daemon configuration file at '/var/run/jami/session-local.conf'. (jami-shepherd-services): Add the closure of guile-ac-d-bus and guile-fibers as extensions. Adjust imported modules. Remove no longer used parameters. <jami-dbus-session>: Use a PID file, avoiding the need for the manual synchronization. <jami>: Set DBUS_SESSION_BUS_ADDRESS environment variable. Poll using 'jami-service-available?' instead of 'dbus-service-available?'. * gnu/tests/telephony.scm (run-jami-test): Add needed Guile extensions. Set DBUS_SESSION_BUS_ADDRESS environment variable. Adjust all tests to use 'jami-service-available?' to determine if the service is started rather than the now problematic Shepherd's 'start-service'. Maxim Cournoyer