[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
arsenm wrote: > we might document that those are not supported for now. if users really need > them, introducing more controls to support them Sure. I think it's easiest to make progress if we fix the integer cases as a first step https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
yxsamliu wrote: > > Will the metadata for unsafe-fp-atomics also be controlled by the pragma > > that controls the no-fine-grained and no-remote metadata? e.g. something > > like > > ``` > > #pragma clang atomics begin no-fine-grained(on) no-remote(on) unsafe-fp(on) > > ``` > > Yes, I would expect something like that for consistency. The most problematic > part is the denormal flushing in some address spaces, and secondarily the > fixed rounding mode in other cases we might document that those are not supported for now. if users really need them, introducing more controls to support them https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
arsenm wrote: > Will the metadata for unsafe-fp-atomics also be controlled by the pragma that > controls the no-fine-grained and no-remote metadata? e.g. something like > > ``` > #pragma clang atomics begin no-fine-grained(on) no-remote(on) unsafe-fp(on) > ``` Yes, I would expect something like that for consistency. The most problematic part is the denormal flushing in some address spaces, and secondarily the fixed rounding mode in other cases https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
yxsamliu wrote: > > @arsenm I agree that the default should be assuming fine-grained is > > possible. My thinking behind the original naming and direction was not > > wanting to introduce an unexpected performance regression by default. I'm > > happy for both to be changed, and this patch being rebased on top of #85052 > > once it is merged. > > The opting for fast-and-maybe-broken by default needs to be a frontend > decision (i.e. the language can choose to add the metadata to all atomics). I > believe @yxsamliu is going to be working on the frontend half which will > preserve the HIP behavior > > > One oustanding question for me is, although outside of the scope of this > > PR, how will the original 'no-unsafe-fp' option fit in the new metadata > > node in terms of constraints? Would it imply a new constraint or be covered > > by `no_fine_grained` and `no_remote`? > > I believe we need an additional piece of metadata (which I have another draft > for) to express the relaxable floating point. We can express the > unsafe-fp-math option with the 2 combined, and then can drop the old IR > attribute Will the metadata for unsafe-fp-atomics also be controlled by the pragma that controls the no-fine-grained and no-remote metadata? e.g. something like ``` #pragma clang atomics begin no-fine-grained(on) no-remote(on) unsafe-fp(on) ``` https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
arsenm wrote: > @arsenm I agree that the default should be assuming fine-grained is possible. > My thinking behind the original naming and direction was not wanting to > introduce an unexpected performance regression by default. I'm happy for both > to be changed, and this patch being rebased on top of #85052 once it is > merged. The opting for fast-and-maybe-broken by default needs to be a frontend decision (i.e. the language can choose to add the metadata to all atomics). I believe @yxsamliu is going to be working on the frontend half which will preserve the HIP behavior > > One oustanding question for me is, although outside of the scope of this PR, > how will the original 'no-unsafe-fp' option fit in the new metadata node in > terms of constraints? Would it imply a new constraint or be covered by > `no_fine_grained` and `no_remote`? I believe we need an additional piece of metadata (which I have another draft for) to express the relaxable floating point. We can express the unsafe-fp-math option with the 2 combined, and then can drop the old IR attribute https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
pasaulais wrote: Also, this will need to be extended to other operations that can be impacted by fine-grained and remote access like `add`, `and`, `or`, `sub`, `fpadd` and `fpsub`. https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
pasaulais wrote: > > @arsenm That makes sense, I don't think MMRA fits the fine-grained use case > > either. Does that mean we can stick with the approach from this PR? > > @b-sumner mentioned there was another similar approach being worked on. > > Something like this, but the naming and direction of this PR is backwards. > The default should be assume fine grained memory is possible. We also have > another orthogonal bit we need to track in addition to fine grained memory. I > was envisioning this as a single integer interpreted as bitfields of the two, > but this uses different key:value metadata pairs. It should be named > "amdgpu.no.something" to assert the operation doesn't access fine grained @arsenm I agree that the default should be assuming fine-grained is possible. My thinking behind the original naming and direction was not wanting to introduce an unexpected performance regression by default. I'm happy for both to be changed, and this patch being rebased on top of https://github.com/llvm/llvm-project/pull/85052 once it is merged. One oustanding question for me is, although outside of the scope of this PR, how will the original 'no-unsafe-fp' option fit in the new metadata node in terms of constraints? Would it imply a new constraint or be covered by `no_fine_grained` and `no_remote`? https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
arsenm wrote: > @arsenm That makes sense, I don't think MMRA fits the fine-grained use case > either. Does that mean we can stick with the approach from this PR? @b-sumner > mentioned there was another similar approach being worked on. Something like this, but the naming and direction of this PR is backwards. The default should be assume fine grained memory is possible. We also have another orthogonal bit we need to track in addition to fine grained memory. I was envisioning this as a single integer interpreted as bitfields of the two, but this uses different key:value metadata pairs. It should be named "amdgpu.no.something" to assert the operation doesn't access fine grained https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
pasaulais wrote: > > In this case, MMRAs would only help in the sense that you won't need any > > new attributes and can just add an MMRA such as > > `atomic-lowering:fine-grained`. It's not really what MMRAs were made for > > (because this attribute doesn't affect semantics, just lowering style I > > think?), > > It is semantics. You get undefined behavior if you access fine grained memory > for an atomic where the instruction doesn't handle it. I don't think it quite > fits into the MMRA box, since it isn't about the synchronization properties > of the memory access but where the underlying memory is @arsenm That makes sense, I don't think MMRA fits the fine-grained use case either. Does that mean we can stick with the approach from this PR? @b-sumner mentioned there was another similar approach being worked on. https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
arsenm wrote: > In this case, MMRAs would only help in the sense that you won't need any new > attributes and can just add an MMRA such as `atomic-lowering:fine-grained`. > It's not really what MMRAs were made for (because this attribute doesn't > affect semantics, just lowering style I think?), It is semantics. You get undefined behavior if you access fine grained memory for an atomic where the instruction doesn't handle it. I don't think it quite fits into the MMRA box, since it isn't about the synchronization properties of the memory access but where the underlying memory is https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
Pierre-vh wrote: > Thanks for the comments @arsenm @yxsamliu @b-sumner. > > By approaching a similar solution, do you mean MMRAs (#78569) ? > > If so, should I rebase/adapt my patch to the MMRA PR? Or will this PR be > redundant and needs closing? > > @yxsamliu These concise names look good to me. In this case, MMRAs would only help in the sense that you won't need any new attributes and can just add an MMRA such as `atomic-lowering:fine-grained`. It's not really what MMRAs were made for (because this attribute doesn't affect semantics, just lowering style I think?), but all of the boilerplate would be there to preserve this annotation until the backend, and as long as you don't add any other `atomic-lowering:` flag, you shouldn't have issues with compatibility relations - if you plan to add other flags like that then it may be an issue because you could end up with two operations being considered incompatible (= can reorder them) when they're still technically compatible https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
pasaulais wrote: Thanks for the comments @arsenm @yxsamliu @b-sumner. By approaching a similar solution, do you mean MMRAs (https://github.com/llvm/llvm-project/pull/78569) ? If so, should I rebase/adapt my patch to the MMRA PR? Or will this PR be redundant and needs closing? @yxsamliu These concise names look good to me. https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
b-sumner wrote: We're aware of the concerns regarding atomics handling. We're trying to find the approach which is least distasteful to all parties. I expect we will address the concern raised here, but not necessarily in the same way. https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
yxsamliu wrote: > We're gradually converging on something that looks like this, subject to bike > shedding the name I did not know about this PR. It is interesting that our other discussions lead to similar solution. I agree that per-instruction metadata is needed, and the metadata should convey separate control for fp atomic and unsupported integer atomic across PCIE. The reason to use per-instruction metadata instead of per-function metadata or attribute is that per-function attribute does not work well for inlined functions. As for controlling FE emitting this metadata, a more-fine-grained control e.g. pragma is desirable, however, for this patch, a clang option is probably sufficient. We could consider pragma control later. We need to coin some good concise name for the metadata: For unsafe fp atomic - unsafe_fp ? For unsupported integer atomic across PCIE - unsafe_pcie ? fine_grained may not be suitable since fine_grained memory accessed across XGMI supports integer atomic AND/OR/XOR ops etc https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [AMDGPU] Add an option to disable unsafe uses of atomic xor (PR #69229)
arsenm wrote: We're gradually converging on something that looks like this, subject to bike shedding the name https://github.com/llvm/llvm-project/pull/69229 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
