[PATCH] D157613: [Clang][ExtendLifetimes][3/4] Add -fextend-lifetimes flag to Clang to extend the lifetime of local variables

[Jeremy Morse via Phabricator via cfe-commits]

jmorse added a comment.

The code changes here look good, but it's time to be annoying about the tests -- most of them appear to run the optimisation pipelines, which is testing all of LLVM as well as clang in these tests. Possibly a simple fix to that is to put these in cross-project-tests. Otherwise, I believe these should be only testing the plain codegen output without optimisations at all, i.e. --disable-llvm-passes.



================
Comment at: clang/lib/CodeGen/CGCall.cpp:3492-3499
+        // Ignore fake uses and the instructions that load their
+        // operands.
+        if (Intrinsic->getIntrinsicID() == llvm::Intrinsic::fake_use) {
+          const llvm::Value *FakeUseArg = Intrinsic->getArgOperand(0);
+          if (++II == IE || &*II != FakeUseArg)
+            break;
+          continue;
----------------
It's a bit ugly to switch away from having a range-based loop, am I right in thinking this is because you need to skip the load-to-fake-use? Annoying but necessary I guess. Could I request an explicit "II = std::next(II);" though to really ram home to the reader that we're skipping an extra instruction, using the pre-increment is liable to be missed, and we're not trying to be super-concise here IMO.

The addition of the "I" variable looks like it might be un-necessary, that's only _used_ by GetStoreIfValid which has a domination assignment above it no?


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:1729-1730
+  // Analogous to lifetime markers, we use a 'cleanup' to emit fake.use
+  // calls for local variables. We are exempting volatile variables and
+  // non-scalars larger than 4 times the size of an unsigned int (32 bytes).
+  // Larger non-scalars are often allocated in memory and may create unnecessary
----------------
(This could be even more concise; no strong opinion though).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157613/new/

https://reviews.llvm.org/D157613