Page MenuHomePhabricator

[mlir][OpenMP] Add omp.atomic.update canonicalization
ClosedPublic

Authored by shraiysh on May 27 2022, 5:37 AM.

Details

Summary

This patch adds canonicalization conditions for omp.atomic.update thus
eliminating it when it becomes just a write or a no-op due to other
changes during canonicalization.

Diff Detail

Event Timeline

shraiysh created this revision.May 27 2022, 5:37 AM
shraiysh requested review of this revision.May 27 2022, 5:37 AM

Ping for review.

Ping for review.

Ping for review!

Ping for review.

Thanks @shraiysh for this patch.

"This patch adds canonicalization conditions for omp.atomic.update thus
eliminating it when it becomes just a write or a no-op due to other
changes during canonicalization."

Could you expand this a bit more? Is it write with the same value?

Could you also check whether it is OK to accept atomic updates like the following?

func.func @update_unnecessary_computations(%arg0: memref<i32>) {
  %c0_i32 = arith.constant 0 : i32
  omp.atomic.update   %arg0 : memref<i32> {
  ^bb0(%arg1: i32):
    %0 = arith.cmpi ne, %arg1, %c0_i32 : i32
    %1 = arith.extui %0 : i1 to i32
    %2 = arith.addi %arg1, %1 : i32
    omp.yield(%2 : i32)
  }
  return
}

Could you also check whether it is OK to accept atomic updates like the following?

func.func @update_unnecessary_computations(%arg0: memref<i32>) {
  %c0_i32 = arith.constant 0 : i32
  omp.atomic.update   %arg0 : memref<i32> {
  ^bb0(%arg1: i32):
    %0 = arith.cmpi ne, %arg1, %c0_i32 : i32
    %1 = arith.extui %0 : i1 to i32
    %2 = arith.addi %arg1, %1 : i32
    omp.yield(%2 : i32)
  }
  return
}

This will work as expected - there will be an atomic update to the location with the new value (lowering to LLVM IR will also work as expected). However, I understand that we might not be able to reach this situation because C/C++/Fortran semantic checks will stop it.

mlir/test/Dialect/OpenMP/canonicalize.mlir
16–22

Could you expand this a bit more? Is it write with the same value?

@kiranchandramohan here is an example where an atomic update operation is essentially an atomic write operation. Due to other simplifications during canonicalization of the ops under atomic update region (possibly unrelated to the openmp dialect) like elimination unused instructions etc. - if the IR reaches this state after after them, then this will be replaced with an atomic write operation writing the same value (as can be seen in CHECK lines under this) to the memory.

Ping for review. (This is not urgent for the drop on July 26th, and patches related to those are high priority. This is just a gentle reminder, if anyone is willing to look into this patch. :) )

mehdi_amini added inline comments.Mon, Jul 25, 3:49 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
853

Is there a case for AtomicUpdateOp where yieldOp can have more or less than 1 operand?

858

getFirstOp()?

shraiysh updated this revision to Diff 448002.Wed, Jul 27, 4:40 AM

Addressed comments.

shraiysh marked 2 inline comments as done.Wed, Jul 27, 4:41 AM
shraiysh added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
853

No, removed the check from here. It is already there in the verifier.

ftynse accepted this revision.Wed, Jul 27, 5:46 AM
This revision is now accepted and ready to land.Wed, Jul 27, 5:46 AM
This revision was automatically updated to reflect the committed changes.
shraiysh marked an inline comment as done.