@bondhugula Ready when you are :)

Addressing CR comments

In D86380#2232369, @bondhugula wrote:

Can you please separate out the fix to normalizeAffineParallel? It has really nothing to do with the builder additions.

Thanks for the review everyone, I've made tiny tweaks after further feedback. This includes extracting the core transformation of normalization into a separate utility function (in case other users want to integrate this transformation in a different pass), and also adding a helper method AffineParallelOp::getSteps().

Thanks @ftynse :)

Friendly poke, wondering what the status of this is. Looks like we have an RFC here https://llvm.discourse.group/t/rfc-standard-add-std-fptoui-and-std-uitofp-operations/1567. AFAICT, there aren't any outstanding issues. This is a required op for certain situations. Because the standard dialect only deals with 'signless' integers, we must be able to reintroduce signedness in a few key areas, one of them being casts, otherwise we get incorrect behavior (we've run into this in real-world situations).

In D84998#2218985, @rriddle wrote:

Hey Frank, didn't mean for this to force you to scale back the patch. If it helps I can implement the pattern rewriter hooks that you would need. Just let me know.

• River

After some more discussions and thought, I've removed the complex canonicalizations that:

• break the PatternRewriter rules
• have hacky special exemptions

Looks like xchg can handle floats but not cmpxchg, so I'm in favor of proceeding with this. Might want to also adjust the IR to reflect the tighter constraints.

Additionally, there are some operations that can't feasibly be implemented without floating-point cmpxchg working. I can see wanting to unblock these tests, but without changes to the verification of the GenericAtomicRMWOp, it will be possible for users to create floating-point cmpxchg.

LGTM, thanks!

@dcaballe & @bondhugula Is there anything else you'd like for me to address?

Address comments.

@rriddle I've attempted to follow your advice, could you take another look? Is this what you were thinking of?

Address CR comments

Fix pre-merge lint check

OK, I could see always lowering enums that would lower to cmpxchg via the generic op. But at the moment we have a higher layer that uses the enumerated version for min/max, so I'd prefer to don't remove that. It's much easier to pattern match (even declaratively) the enumerated version than the one with a region.

LGTM

LGTM

LGTM

I don't have MSVC at this minute but this seems OK. Is it that the older compiler doesn't understand constexpr functions? I guess there's no fundamental reason the accessor operators need to be constexpr.

arc?

Fix formatting

Use git-clang-format

I tried to use this from outside of MLIR core and it ended up not possible to even call the getFlattendAffineExprs that doesn't take a 3rd argument due to that being ambiguous with the one defined in AffineStructures.

See https://reviews.llvm.org/D75584 for a potential fix.

• Merge branch 'master' into arcpatch-D74401
• Merge branch 'master' into arcpatch-D74401
• Update with master

@rriddle Thanks for the review, does this look better now?

• Fix example

• Review feedback

• Fix example

• Comments

Simplified design based on RFC feedback

This is great! Would it be possible to parse keywords into enum attrs as well?

Just throwing an idea out there, but what if instead of 'named' ops (which seems to be the source of the need to extend TableGen), we could use something like the 'contraction' in PlaidML's tile dialect. It's not as flexible as arbitrary generic ops, but it seems to cover most of the ML workloads we've thrown at it. Here's what the ContractionOp looks like: https://github.com/plaidml/plaidml/blob/plaidml-v1/pmlc/dialect/tile/ir/ops.td#L232 (you can ignore the other ops in there, they are meant for the construction phase, for turning values into the type system via attributes/AffineMaps, etc). I'm thinking we really only need 2 kinds of ops: Contraction op and a generic Elementwise op. Bonus: both of these forms are auto-differentiable. We could add the other properties of structured ops.

• Remove stdx.
• Review updates
• Remove iv
• Review updates
• Drop trivial braces
• Trivial braces
• Address feedback

@rriddle Are there anymore blockers that need to be addressed?

Rebase

• stream

• - Review updates

@rriddle Are you OK with the latest changes? Thanks :)

• Drop trivial braces
• Trivial braces

• Review updates

Review addressed in latest push.

• Remove iv

• Remove iv