Add WriteAfterWriteElimination pass #2572

annagrin · 2025-02-01T00:08:46Z

Description

Add a pass for removing stores that are overridden by other stores

Example:

func.func @test_two_stores_same_pointer() {
  %c0_i64 = arith.constant 0 : i64
  %0 = quake.alloca !quake.veq<2>
  %1 = cc.const_array [1] : !cc.array<i64 x 1>
  %2 = cc.extract_value %1[0] : (!cc.array<i64 x 1>) -> i64
  %3 = cc.alloca !cc.array<i64 x 1>
  %4 = cc.cast %3 : (!cc.ptr<!cc.array<i64 x 1>>) -> !cc.ptr<i64>
  cc.store %c0_i64, %4 : !cc.ptr<i64>
  cc.store %2, %4 : !cc.ptr<i64>
  %5 = cc.load %4 : !cc.ptr<i64>
  %6 = quake.extract_ref %0[%5] : (!quake.veq<2>, i64) -> !quake.ref
  quake.x %6 : (!quake.ref) -> ()
  return
}

The in the last two stores, the first one can be removed because it is overridden by the next.

  cc.store %c0_i64, %4 : !cc.ptr<i64>
  cc.store %2, %4 : !cc.ptr<i64>

Signed-off-by: Anna Gringauze <agringauze@nvidia.com>

github-actions · 2025-02-01T01:38:57Z

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

iqbalbhatti49

Review Summary:

The code looks great overall! You've followed best practices, and the implementation is clean and efficient. The logic is sound, and I appreciate the attention to detail in the structure.

Highlights:

Code is well-organized and easy to follow.
Proper handling of edge cases.
Clear variable naming makes the code readable.

Overall, I approve this change and recommend merging it. Great work!

…eless-stores Signed-off-by: Anna Gringauze <agringauze@nvidia.com>

github-actions · 2025-02-02T02:56:23Z

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

schweitzpgi

"Useless" sounds a little judgmental or something to me. Maybe WAW elimination or WAW removal sounds less aggressive? heh

schweitzpgi · 2025-02-03T18:24:13Z

include/cudaq/Optimizer/Transforms/Passes.td

@@ -975,6 +975,29 @@ def RegToMem : Pass<"regtomem", "mlir::func::FuncOp"> {
  let dependentDialects = ["cudaq::cc::CCDialect", "quake::QuakeDialect"];
 }

+def RemoveUselessStores : Pass<"remove-useless-stores", "mlir::func::FuncOp"> {


I think we should call this "write-after-write-elimination`.

We can also make it a generic pass, since we may want to run it on more than func.func.

schweitzpgi · 2025-02-03T18:25:44Z

include/cudaq/Optimizer/Transforms/Passes.td

+def RemoveUselessStores : Pass<"remove-useless-stores", "mlir::func::FuncOp"> {
+  let summary = "Remove stores that are overriden by other stores.";
+  let description = [{
+


Can we add more words here? I think the example is good, but examples aren't the best documentation as they can leave some unanswered questions.

schweitzpgi · 2025-02-03T18:37:53Z

lib/Optimizer/Transforms/RemoveUselessStores.cpp

+  // /// Detect if value is used in the op or its nested blocks.
+  // static bool isUsed(Value v, Operation *op) {
+  //   for (auto opnd : op->getOperands())
+  //     if (opnd == v)
+  //       return true;
+
+  //   for (auto &region : op->getRegions())
+  //     for (auto &b : region)
+  //       for (auto &innerOp : b)
+  //         if (isUsed(v, &innerOp))
+  //           return true;
+
+  //   return false;
+  // }


schweitzpgi · 2025-02-03T18:38:30Z

lib/Optimizer/Transforms/RemoveUselessStores.cpp

+          // // Found a use for a current ptr before the overriding store
+          // if (currentPtr && isUsed(currentPtr, &op))
+          //   return false;
+        }
+      }
+      // } else {
+      //   // Found a use for a current ptr before the overriding store
+      //   if (currentPtr && isUsed(currentPtr, &op))
+      //     return false;
+      // }


schweitzpgi · 2025-02-03T18:40:46Z

lib/Optimizer/Transforms/RemoveUselessStores.cpp

+    if (auto alloca = ptrOp.getDefiningOp<cudaq::cc::AllocaOp>())
+      return true;
+
+    return false;


Suggested change

if (auto alloca = ptrOp.getDefiningOp<cudaq::cc::AllocaOp>())

return true;

return false;

return isa_and_present<cudaq::cc::AllocaOp>(ptrOp.getDefiningOp());

schweitzpgi · 2025-02-03T18:54:47Z

lib/Optimizer/Transforms/RemoveUselessStores.cpp

+
+    auto block = store.getOperation()->getBlock();
+    for (auto &op : *block) {
+      if (auto currentStore = dyn_cast<cudaq::cc::StoreOp>(&op)) {


I was thinking of doing this with an analysis step, first.

Have a DenseMap<Block*, DenseMap<Value, SmallVector<cudaq::cc::StoreOp*>>>
Scan the current operation's regions for cc::StoreOps. For each one found, add it to the map object by its block and ptr value argument.

After that go back over the map of maps, see if any of the vectors have > 1 element, and use dominance info to make sure no uses of the ptr value are dominated by the dominant store and dominate the post-dominant store other than any other stores in the block between the two (most dominant, most postdominant). Dominance here will catch uses into, e.g., cc::IfOp, etc.

If there is nothing but stores in the block between the two endpoint stores, go ahead and remove all but the postdominant one.

A more aggressive check would be to eliminate the dominant stores in a pair-wise, incremental approach. So if there is a store, if(with a load), store[*], store , the dead store[*] would be eliminated even though the first one cannot be removed.

I'll try that, thanks! I do need the most aggressive approach it seems from the uccsd examples

Signed-off-by: Anna Gringauze <agringauze@nvidia.com>

Add RemoveUselessStores pass

9b55600

Signed-off-by: Anna Gringauze <agringauze@nvidia.com>

annagrin mentioned this pull request Feb 1, 2025

Make sure uccsd from cudaqx compiles for quantum devices #2458

Draft

github-actions bot pushed a commit that referenced this pull request Feb 1, 2025

Docs preview for PR #2572.

08c1c98

iqbalbhatti49 approved these changes Feb 1, 2025

View reviewed changes

annagrin requested a review from schweitzpgi February 2, 2025 01:23

Merge branch 'main' of https://github.com/NVIDIA/cuda-quantum into us…

0b739e8

…eless-stores Signed-off-by: Anna Gringauze <agringauze@nvidia.com>

github-actions bot pushed a commit that referenced this pull request Feb 2, 2025

Docs preview for PR #2572.

5b30ec8

schweitzpgi reviewed Feb 3, 2025

View reviewed changes

Address some CR comments

3393f87

Signed-off-by: Anna Gringauze <agringauze@nvidia.com>

copy-pr-bot bot temporarily deployed to default February 3, 2025 20:41 Inactive

copy-pr-bot bot temporarily deployed to default February 3, 2025 20:42 Inactive

copy-pr-bot bot temporarily deployed to default February 3, 2025 20:43 Inactive

annagrin changed the title ~~Add RemoveUselessStores pass~~ Add WriteAfterWriteElimination pass Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WriteAfterWriteElimination pass #2572

Add WriteAfterWriteElimination pass #2572

annagrin commented Feb 1, 2025 •

edited

Loading

github-actions bot commented Feb 1, 2025

iqbalbhatti49 left a comment

github-actions bot commented Feb 2, 2025

schweitzpgi left a comment

schweitzpgi Feb 3, 2025

schweitzpgi Feb 3, 2025

schweitzpgi Feb 3, 2025

schweitzpgi Feb 3, 2025

schweitzpgi Feb 3, 2025

schweitzpgi Feb 3, 2025

annagrin Feb 3, 2025 •

edited

Loading

Add WriteAfterWriteElimination pass #2572

Are you sure you want to change the base?

Add WriteAfterWriteElimination pass #2572

Conversation

annagrin commented Feb 1, 2025 • edited Loading

Description

github-actions bot commented Feb 1, 2025

iqbalbhatti49 left a comment

Choose a reason for hiding this comment

Review Summary:

Highlights:

github-actions bot commented Feb 2, 2025

schweitzpgi left a comment

Choose a reason for hiding this comment

schweitzpgi Feb 3, 2025

Choose a reason for hiding this comment

schweitzpgi Feb 3, 2025

Choose a reason for hiding this comment

schweitzpgi Feb 3, 2025

Choose a reason for hiding this comment

schweitzpgi Feb 3, 2025

Choose a reason for hiding this comment

schweitzpgi Feb 3, 2025

Choose a reason for hiding this comment

schweitzpgi Feb 3, 2025

Choose a reason for hiding this comment

annagrin Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

annagrin commented Feb 1, 2025 •

edited

Loading

annagrin Feb 3, 2025 •

edited

Loading