[PATCH] D133310: [Assignment Tracking][15/*] Account for assignment tracking in simplifycfg

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Soooo, for the `TryToSimplifyUncondBranchFromEmptyBlock` case I think this would expose the "LLVM can't represent things that happen on critical edges" issue [0], in that sinking the dbg.assigns out of a trivial block might put them in a block with other predecessors. IMO, it's preferable to maintain what dbg.values currently do, which is to vanish (sometimes causing wrong debug-info to be produced), because it's not a regression.

Addressing this is a much larger issue; possibly we can deal with that in the future, but I'd suggest not as part of this patch series. Same with the "hoist all..." situation -- that might be an improvement, but we should start off with no regressions, then consider the improvements we can make.

~

For the store-speculation that happens -- should there not be a call somewhere to merge the assign-IDs of these stores? (Possibly not). Plus, is there a possibility / chance that

- stores are duplicated for some reason (unrolling?) and dbg.assigns with them, using different values,
- one of them is speculated in `SpeculativelyExecuteBB`,
- We end up setting the Value operand of all the dbg.assigns with the assign ID, to the speculated value, where some of them referred to a non-speculated value.

Maybe there's a reason why this doesn't happen. If it does, I suppose all that needs to happen is the assignment markers are filtered by whether their Value is the same as the speculated stores value.

(Note to self, didn't look at tests)

[0] https://github.com/llvm/llvm-project/issues/55115#issuecomment-1118420725



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1897-1898
+      continue;
+    // The remaining uses are debug users, replace those with the common inst.
+    // In most (all?) cases this just introduces a use-before-def.
+    I->replaceAllUsesWith(I0);
----------------
This can (should!) be asserted; also, techncially can be peeled away from dbg.assign improvements if this also affects dbg.values?


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

https://reviews.llvm.org/D133310