[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm closed this revision. arsenm added a comment. bc37be1855773c1dcf8c6bf577a096a81fd58652 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142907/new/ https://reviews.llvm.org/D142907 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. Oh, sorry, I missed that the new code specifically runs on functions imported using -mlink-builtin-bitcode. I somehow thought it was running on all functions. LGTM CHANGES SINCE LAST A

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm added a comment. In D142907#4288555 , @efriedma wrote: > If you have a library function that's built with > "denormal-fp-math"="dynamic,dynamic", you can link it into code built in any > mode, and LTO should be able to propagate that mode from th

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

efriedma added a comment. If you have a library function that's built with "denormal-fp-math"="dynamic,dynamic", you can link it into code built in any mode, and LTO should be able to propagate that mode from the caller to the callee. That doesn't require clang to do anything special; you can

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm added a comment. In D142907#4288247 , @efriedma wrote: > Given that, I don't follow the whole "merging" thing... we should just be > setting whatever mode is active. The attribute setting should not depend on > whether the function is interposab

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

efriedma added a comment. I'm having trouble understanding the changes on the clang side. If I'm following correctly; the "denormal-fp-math" setting is a promise from the user to the compiler: if the setting is not "dynamic", the user promises that the definition will only execute in the specif

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

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

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

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

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

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

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

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

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

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

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm updated this revision to Diff 505274. arsenm marked an inline comment as done. arsenm added a comment. Update doxygen comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142907/new/ https://reviews.llvm.org/D142907 Files: clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/Code

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

jcranmer-intel added a comment. I'm studiously ignoring the Clang and LLVM codegen changes here, but otherwise, I think the direction of this change is generally good. Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1377-1378 llvm_unreachable("unknown denormal mode"); -

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

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

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm added a comment. In D142907#4132836 , @jcranmer-intel wrote: > In D142907#4132430 , @arsenm wrote: > >> I was thinking of changing the default in general to dynamic. I was going to >> at least change the

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm added inline comments. Comment at: llvm/docs/LangRef.rst:2166 + +If the mode is ``"dynamic"``, the behavior is derived from the +dynamic state of the floating-point environment. Transformations pengfei wrote: > 1. Does it mean users must specify `d

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

pengfei added inline comments. Comment at: llvm/docs/LangRef.rst:2166 + +If the mode is ``"dynamic"``, the behavior is derived from the +dynamic state of the floating-point environment. Transformations 1. Does it mean users must specify `dynamic` when the

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

jcranmer-intel added a comment. In D142907#4132430 , @arsenm wrote: > I was thinking of changing the default in general to dynamic. I was going to > at least change the strictfp default in a follow up I had the same thought too, but I reflected a little

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm added a comment. In D142907#4132543 , @kpn wrote: > What's the plan for tying this to strictfp? Because I don't it should be tied > to cases where we use the constrained intrinsics but the exceptions are > ignored and the default rounding is in s

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

kpn added a comment. What's the plan for tying this to strictfp? Because I don't it should be tied to cases where we use the constrained intrinsics but the exceptions are ignored and the default rounding is in stated. Those instructions are supposed to behave the same as the non-constrained ins

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm added a comment. In D142907#4132318 , @jcranmer-intel wrote: > Not entirely sure where the best place to effect this (I think somewhere in > the clang driver code?), but on further reflection, it feels like strict > fp-model in clang should set

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

jcranmer-intel added a comment. Not entirely sure where the best place to effect this (I think somewhere in the clang driver code?), but on further reflection, it feels like strict fp-model in clang should set the denormal mode to dynamic. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm added a comment. In D142907#4119339 , @andrew.w.kaylor wrote: > In general, it seems like the denormal mode should be considered part of the > floating point environment (though as far as I know the C standard, at least, > doesn't document it as

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

andrew.w.kaylor added a comment. In general, it seems like the denormal mode should be considered part of the floating point environment (though as far as I know the C standard, at least, doesn't document it as such). If it were considered part of the floating point environment, the LLVM rules

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm updated this revision to Diff 494465. arsenm marked 2 inline comments as done. arsenm added a comment. Documentation fixes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142907/new/ https://reviews.llvm.org/D142907 Files: clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/CodeGen

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

jcranmer-intel added a comment. Looking at the attribute logic here, there is conceptual room for both a `dynamic` and an `unknown` mode (i.e., you get a top and a bottom value), but I don't think there is value in distinguishing between them, so I'm fine with keeping just a `dynamic`. I didn'

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm updated this revision to Diff 494417. arsenm edited the summary of this revision. arsenm added a comment. Herald added a subscriber: tpr. Address comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142907/new/ https://reviews.llvm.org/D142907 Files: clang/lib/CodeGen/CGCall

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

kpn added inline comments. Comment at: llvm/test/Transforms/Inline/AMDGPU/inline-denormal-fp-math.ll:78 +; CHECK-NEXT:[[CALL:%.*]] = call i32 @func_default() +; CHECK-NEXT:ret i32 [[CALL]] ; arsenm wrote: > kpn wrote: > > Are we changing the behavior in

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm marked an inline comment as done. arsenm added inline comments. Comment at: llvm/test/Transforms/Inline/AMDGPU/inline-denormal-fp-math.ll:78 +; CHECK-NEXT:[[CALL:%.*]] = call i32 @func_default() +; CHECK-NEXT:ret i32 [[CALL]] ; kpn wrote: > Are we

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

kpn added a comment. We use "dynamic" for the constrained intrinsics. I'd stay consistent with our terminology and stick with "dynamic" here. I like the amount of testing. You may have gotten every single combination of cases, but I didn't go far enough to check. Comment at:

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

tra added a comment. LGTM for the parts I've commented on. Comment at: clang/test/CodeGenCUDA/link-builtin-bitcode-denormal-fp-mode.cu:77 + +// CHECK: kernel_f32({{.*}}) #[[$KERNELATTR:[0-9]+]] +__global__ void kernel_f32(float* out, float* a, float* b, float* c) {

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm updated this revision to Diff 493653. arsenm added a comment. Fix dropping target-cpu. Also skip interposable functions if we aren't internalizing (this seems to be a theoretical concern, since PropagateAttrs and Internalize are set as a pair) CHANGES SINCE LAST ACTION https://reviews

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm updated this revision to Diff 493639. arsenm added a comment. Fix losing target-cpu CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142907/new/ https://reviews.llvm.org/D142907 Files: clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/CodeGenAction.cpp clang/lib/CodeGen/CodeGenM

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm marked an inline comment as done. arsenm added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2059-2061 + F.removeFnAttrs(AttrsToRemove); + addDenormalModeAttrs(Merged, MergedF32, FuncAttrs); + F.addFnAttrs(FuncAttrs); arsenm wrote: > tra wro

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm marked 2 inline comments as done. arsenm added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2059-2061 + F.removeFnAttrs(AttrsToRemove); + addDenormalModeAttrs(Merged, MergedF32, FuncAttrs); + F.addFnAttrs(FuncAttrs); tra wrote: > IIUIC, thi

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

tra added a comment. My $.02, mostly on the style and the mechanics of applying the new attribute. FP semantics aspects are above my pay grade. Comment at: clang/lib/CodeGen/CGCall.cpp:2059-2061 + F.removeFnAttrs(AttrsToRemove); + addDenormalModeAttrs(Merged, MergedF32, Func

[PATCH] D142907: LangRef: Add "dynamic" option to "denormal-fp-math"

arsenm created this revision. arsenm added reviewers: scanon, spatel, cameron.mcinally, andrew.w.kaylor, tra, jlebar, Anastasia, yaxunl, efriedma, jcranmer-intel, kpn, sepavloff. Herald added subscribers: kosarev, foad, StephenFan, wenlei, jdoerfert, kerbowa, pengfei, hiraditya, jvesely. Herald a