I just assume there are more devices out there that we don't know about or understand.

Thanks for the review! Replied to a few things inline as I continue to work on this.

Since hasNoEffect did not walk the op regions and isMemoryEffectFree does, I think with this change we have a couple of redundant checks on the attached regions.

In D136376#3892425, @chelini wrote:

Thanks to me looks good. But please have a look at the build status, as it is currently failing.

• Address review

Friendly ping!

• Address review

• Add disclaimer about graph regions

Also we should link to this doc from other places (like the SideEffect interface doc).

• reformat
• address review comments

• Use m_Constant
• Change getConstantStep() to return APInt (that's more robust)
• Refactor one obvious place in the code to use ForOp::getConstantStep()

• Add note about Pure

In D135505#3860870, @lattner wrote:

Did you consider something more explicit and less loaded with terminology baggage?

Sent https://reviews.llvm.org/D135899 adding the llvm_unreachable.

In D135505#3855933, @NathanielMcVicar wrote:

There is also a failure in the MLIR Windows bot. https://lab.llvm.org/buildbot/#/builders/13/builds/27028

C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Transforms\Utils\SideEffectUtils.cpp(58) : error C2220: the following warning is treated as an error
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\lib\Transforms\Utils\SideEffectUtils.cpp(58) : warning C4715: 'mlir::isSpeculatable': not all control paths return a value

Address review

In D135505#3853519, @clementval wrote:

In D135505#3853489, @mehdi_amini wrote:

In D135505#3853465, @clementval wrote:

Why this patch landed with obvious failures in the pre merge checks? https://buildkite.com/llvm-project/premerge-checks/builds/116406#0183ca8c-bc14-4314-9b1d-c764cf47e0aa

The premiere checks aren't "visible enough": I never notice them myself (I use to when they were posting the results in a comment initially).

Please revert if a fix forward isn't straightforward (should be a sed in flang I think?)

Fixed in e4ccba1598c07bc1fb6f3b73942dbc0a03fa51ed

Thanks @clementval!!

• Fix a use of RecursiveSideEffects.

I didn't notice the failure either. A comment would be more visible.

Address review

In D135505#3848200, @mehdi_amini wrote:

Technically the "safe" update is s/NoSideEffect /NoMemoryEffect/

(I agree that most of the time they want Pure instead)

In D135505#3847282, @zero9178 wrote:

I am indifferent about that, but do wonder whether it is somewhat out of scope for this patch and requires more discussion. It is renaming one of the (if not the) most used trait after all, and its not obvious for downstream users looking through the commit title that such a rename is required.

• Remove code that TableGen generates anyway

• remove stray whitespace

Where is this? I would have perhaps expected it in Rationale.md

Remove stray comment

@zero9178: incorporated your suggestion, PTAL.

• fix comment

Addressed review comments:

Just making sure I understand -- are you saying that MemAlloc is speculatable but still needs a non-trivial MemoryEffect because it allocates memory? Now that I think of it, another such example is a load from a global variable. Since the global variable is deferenceable for the lifetime of the program, the load itself is speculatable, but we still need to model the read memory effect to make sure we don't reorder it with aliasing writes.

• No need for extraClassDeclaration in always_speculatable_op

In D118543#3282422, @bondhugula wrote:

Can you please add a commit summary?

Address comments.