[PATCH] D90174: [HIP] Fix regressions due to fp contract change

This revision was automatically updated to reflect the committed changes. Closed by commit rGcb08558caa3b: [HIP] Fix regressions due to fp contract change (authored by yaxunl). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon accepted this revision. scanon added a comment. This revision is now accepted and ready to land. I'm fine with this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl updated this revision to Diff 304604. yaxunl edited the summary of this revision. yaxunl added a comment. rename faststd to fast-honor-pragmas CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 Files: clang/docs/LanguageExtensions.rst

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Probably should be pluralized for consistency, `fast-honor-pragmas`, but yeah, that's fine with me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. In D90174#2389269 , @tra wrote: > In D90174#2387518 , @scanon wrote: > >> Strictly speaking, fp-contract=fast probably should have been a separate >> flag entirely (since there's no

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

tra added a comment. In D90174#2387518 , @scanon wrote: > Strictly speaking, fp-contract=fast probably should have been a separate flag > entirely (since there's no _expression_ being contracted in fast). > Unfortunately, that ship has sailed, and it

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. Strictly speaking, fp-contract=fast probably should have been a separate flag entirely (since there's no _expression_ being contracted in fast). Unfortunately, that ship has sailed, and it does constrain our ability to choose an accurate name somewhat. What if we just

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. `fast-strict`? Sounds sort of oxymoronic, but it's `fast` while also being strict about honoring pragmas. I don't have any great ideas here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. How about fast-constrained, fast-limited, fast-restricted, or fast-restrained? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. I do not much like faststd, as there's nothing "standard" about it. I do not, however, have a better suggestion off the top of my head. Let's pause and consider the name a little bit longer, please? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Okay. It sounds like strict compatibility with GCC implies ignoring pragmas in fast, and that's what we're most concerned with, since this is originally a GCC option. So the only question I have now is whether `faststd` is the best name for this. Does anyone want

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. @rjmccall ping. Any further concerns for this patch? Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. In D90174#2373829 , @yaxunl wrote: > In D90174#2371577 , @rjmccall wrote: > >> Hmm. Do we actually want this behavior of `fast` overriding pragmas? What >> do other compilers do here? It

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. In D90174#2371577 , @rjmccall wrote: > Hmm. Do we actually want this behavior of `fast` overriding pragmas? What > do other compilers do here? It might be reasonable to just treat this as a > bug. I think clang is just trying

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl updated this revision to Diff 302662. yaxunl marked 7 inline comments as done. yaxunl added a comment. revised manual by John's comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 Files: clang/docs/LanguageExtensions.rst

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. (If you tell GCC to respect the pragma via -std=c17 or similar, then -ffp-contract=fast overrides it just like clang's current behavior: https://godbolt.org/z/5dxxGb) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

scanon added a comment. GCC doesn't respect the pragma, so "what other compilers do" is not a particularly useful metric. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Hmm. Do we actually want this behavior of `fast` overriding pragmas? What do other compilers do here? It might be reasonable to just treat this as a bug. Comment at: clang/docs/LanguageExtensions.rst:3214 +should be noted that

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl updated this revision to Diff 302560. yaxunl added a comment. updated manual CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 Files: clang/docs/LanguageExtensions.rst clang/docs/UsersManual.rst

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. In D90174#2370336 , @rjmccall wrote: > I agree this is useful. However, you need to update the manual to cover > `faststd`. will update the manual. Comment at: clang/test/CodeGenCUDA/fp-contract.cu:203 + +//

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. I agree this is useful. However, you need to update the manual to cover `faststd`. Could you also an IRGen test for this rather than only testing CUDA assembly output? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

tra added a comment. LGTM, but I'll defer to @rjmccall for the approval. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl updated this revision to Diff 302099. yaxunl edited the summary of this revision. yaxunl added a comment. Herald added subscribers: dexonsmith, dang. introduce faststd as value for -ffp-contract and use it for HIP by default. CHANGES SINCE LAST ACTION

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

tra added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + yaxunl wrote: > rjmccall wrote: > > tra wrote: > > > yaxunl wrote: > > > > tra wrote: > > > > > yaxunl

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + rjmccall wrote: > tra wrote: > > yaxunl wrote: > > > tra wrote: > > > > yaxunl wrote: > > > > > tra

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + tra wrote: > yaxunl wrote: > > tra wrote: > > > yaxunl wrote: > > > > tra wrote: > > > > > I don't

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

tra added a subscriber: scanon. tra added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + yaxunl wrote: > tra wrote: > > yaxunl wrote: > > > tra wrote: > > > > I

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + tra wrote: > yaxunl wrote: > > tra wrote: > > > I don't think it's a good idea to force this. > > >

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

tra added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + yaxunl wrote: > tra wrote: > > I don't think it's a good idea to force this. > > > > Perhaps a better

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Argh, sorry! I meant to say "I have *no* objections". CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + tra wrote: > I don't think it's a good idea to force this. > > Perhaps a better way to address this

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

tra added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + I don't think it's a good idea to force this. Perhaps a better way to address this would be to set

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl added a comment. In D90174#2354249 , @rjmccall wrote: > I have objections to the code change here. I'll leave the conceptual > question to other people interested in the HIP toolchain. Is it OK to introduce a clang codegen option e.g.

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. I have objections to the code change here. I'll leave the conceptual question to other people interested in the HIP toolchain. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

yaxunl created this revision. yaxunl added reviewers: tra, rjmccall. Herald added a subscriber: tpr. yaxunl requested review of this revision. Recently HIP toolchain made a change to use clang instead of opt/llc to do compilation (https://reviews.llvm.org/D81861). The intention is to make HIP