Page MenuHomePhabricator

[mlir] do not use llvm.cmpxchg with floats
ClosedPublic

Authored by ftynse on Aug 13 2020, 5:47 AM.

Details

Summary

According to the LLVM Language Reference, 'cmpxchg' accepts integer or pointer
types. Several MLIR tests were using it with floats as it appears possible to
programmatically construct and print such an instruction, but it cannot be
parsed back. Use integers instead.

Depends On D85899

Diff Detail

Event Timeline

ftynse created this revision.Aug 13 2020, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2020, 5:47 AM
ftynse requested review of this revision.Aug 13 2020, 5:47 AM
rriddle accepted this revision.Aug 13 2020, 11:29 AM
This revision is now accepted and ready to land.Aug 13 2020, 11:29 AM
jfb added a comment.Aug 13 2020, 11:31 AM

I'd expect us to go the other way, and allow this. There have been patches in the past to allow usage of atomics with float. Could you look with their authors and see what they advise here?

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.

ftynse added a comment.EditedAug 13 2020, 2:55 PM
In D85900#2216323, @jfb wrote:

I'd expect us to go the other way, and allow this. There have been patches in the past to allow usage of atomics with float. Could you look with their authors and see what they advise here?

LLVM is inconsistent about this: LangRef says integers and pointers are allowed, and one cannot parse cmpxchg with floats, but one apparently _can_ build such instruction. I brought it up here http://lists.llvm.org/pipermail/llvm-dev/2020-August/144275.html, let's continue there. I prefer MLIR to be conservative.

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.

We need some digging at how much is this actually supported in LLVM. I haven't had time so far, help appreciated.

If GenericAtomicRMWOp produces is rewritten to ops that fail the verifier, the conversion will fail, we will not have IR with invalid ops in it.

jfb added a subscriber: reames.Aug 13 2020, 3:00 PM

See for example r360421 D58251 D50491 D26266 D26266 r264845 D15471
Maybe talk to folks such as @reames ?

In D85900#2216949, @jfb wrote:

See for example r360421 D58251 D50491 D26266 D26266 r264845 D15471
Maybe talk to folks such as @reames ?

Can we have this discussion on llvm-dev, http://lists.llvm.org/pipermail/llvm-dev/2020-August/144275.html, rather than in a MLIR review thread that I assume most relevant folks won't follow?

I feel quite strongly against allowing MLIR LLVM dialect to directly contradict LLVM IR spec. https://llvm.org/docs/LangRef.html#id220 says "The type of ‘<cmp>’ must be an integer or pointer type whose bit width is a power of two greater than or equal to eight and less than or equal to a target-specific size limit", which should be reflected as AnyTypeOf<[LLVM_AnyPow2Integer, LLVM_AnyPointer]>; in MLIR LLVM op definition. If LLVM IR spec changes, I am happy to reflect that in MLIR op definition, it's a one-liner change. Note that it does not preclude other dialects, including those that model LLVM IR intrinsics, from having any operations they want, they just shouldn't pretend they map LLVM IR into an MLIR dialect.

flaub accepted this revision.Aug 14 2020, 10:32 AM

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.

See: https://lists.llvm.org/pipermail/llvm-dev/2020-August/144300.html

This revision was automatically updated to reflect the committed changes.