[llvm-branch-commits] [clang] [KeyInstr][Clang] Switch stmt atom (PR #134643)

[Orlando Cazalet-Hyams via llvm-branch-commits]

================
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -gkey-instructions -x c++ -std=c++17 %s -debug-info-kind=line-tables-only -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank --check-prefixes=CHECK,CHECK-CXX
+
+// RUN: %clang_cc1 -gkey-instructions -x c %s -debug-info-kind=line-tables-only -emit-llvm -o - \
+// RUN: | FileCheck %s --implicit-check-not atomGroup --implicit-check-not atomRank
+
+int g;
+void a(int A, int B) {
+// CHECK: entry:
+// The load gets associated with the branch rather than the store.
+// FIXME: Is that the best thing to do?
----------------
OCHyams wrote:

Consider also that we could have:
```
switch
  ((g = A))
{
```

If the store gets DSE'd and switch stays put, and we have the current setup (load in switch's atom group), then we don't step on `((g = A))`. The store and load are line `y` and switch is line `x`. So non-key-instructions behaviour in that case is step `y, x` whereas with this patch it would be just `x`. If we associated the load with the store instead of the switch it'd be `y, x` (step on the load then the switch/br).

That's a long way of saying... maybe we _should_ switch it round? I can't remember how tricky it would be. I think at worst it involves adding another param to `addInstToNewSourceAtom` to avoid overriding existing groups (atm it won't override an existing group if the new rank designation is lower priority.

We could make that `<=` instead too... that would impact the other dual-membership setups which we need to check all make sense after the change.

So - any objection to me adding this to the TODO list instead, to submit it as another patch after the initial stack lands?



https://github.com/llvm/llvm-project/pull/134643