[PATCH] D59795: [DAGCombine] Improve Lifetime node chains.

Wed Mar 27 05:01:41 PDT 2019

courbet added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15585
-    Chains.pop_back();
-    if (!Chain.hasOneUse())
-      continue;
----------------
niravd wrote:
> courbet wrote:
> > I have a hard time convincing myself that we can omit this check. Can you add a comment to the code to explain why ?
> It's not strictly; we could accidentally skip a store because the TokenFactors aren't properly minimized. There needs to be an extra check that the found store is the last semantically meaningful before the LIFETIME_END. I'll factor this out separately from the chain improvement logic. 
> we could accidentally skip a store because the TokenFactors aren't properly minimized.

What happens then ? :)

Can you add this comment to the code so that future readers are aware of the choices made here ?

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1600
+  case ISD::LIFETIME_START:
+    return visitLIFETIME_START(N);
   case ISD::LIFETIME_END:       return visitLIFETIME_END(N);
----------------
Maybe avoid clang-formatting this for consistency with the rest of the function ? (sigh...)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59795