[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
This revision was automatically updated to reflect the committed changes. Closed by commit rL283251: [clang] make reciprocal estimate codegen a function attribute (authored by spatel). Changed prior to commit: https://reviews.llvm.org/D24815?vs=73364=73548#toc Repository: rL LLVM https://reviews.llvm.org/D24815 Files: cfe/trunk/lib/CodeGen/BackendUtil.cpp cfe/trunk/lib/CodeGen/CGCall.cpp cfe/trunk/test/CodeGen/attr-mrecip.c Index: cfe/trunk/test/CodeGen/attr-mrecip.c === --- cfe/trunk/test/CodeGen/attr-mrecip.c +++ cfe/trunk/test/CodeGen/attr-mrecip.c @@ -0,0 +1,7 @@ +// RUN: %clang_cc1 -mrecip=!sqrtf,vec-divf:3 -emit-llvm %s -o - | FileCheck %s + +int baz(int a) { return 4; } + +// CHECK: baz{{.*}} #0 +// CHECK: #0 = {{.*}}"reciprocal-estimates"="!sqrtf,vec-divf:3" + Index: cfe/trunk/lib/CodeGen/CGCall.cpp === --- cfe/trunk/lib/CodeGen/CGCall.cpp +++ cfe/trunk/lib/CodeGen/CGCall.cpp @@ -1730,6 +1730,9 @@ FuncAttrs.addAttribute("no-trapping-math", llvm::toStringRef(CodeGenOpts.NoTrappingMath)); + +// TODO: Are these all needed? +// unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags. FuncAttrs.addAttribute("no-infs-fp-math", llvm::toStringRef(CodeGenOpts.NoInfsFPMath)); FuncAttrs.addAttribute("no-nans-fp-math", @@ -1746,6 +1749,12 @@ "correctly-rounded-divide-sqrt-fp-math", llvm::toStringRef(CodeGenOpts.CorrectlyRoundedDivSqrt)); +// TODO: Reciprocal estimate codegen options should apply to instructions? +std::vector = getTarget().getTargetOpts().Reciprocals; +if (!Recips.empty()) + FuncAttrs.addAttribute("reciprocal-estimates", + llvm::join(Recips.begin(), Recips.end(), ",")); + if (CodeGenOpts.StackRealignment) FuncAttrs.addAttribute("stackrealign"); if (CodeGenOpts.Backchain) Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp === --- cfe/trunk/lib/CodeGen/BackendUtil.cpp +++ cfe/trunk/lib/CodeGen/BackendUtil.cpp @@ -533,9 +533,6 @@ llvm::TargetOptions Options; - if (!TargetOpts.Reciprocals.empty()) -Options.Reciprocals = TargetRecip(TargetOpts.Reciprocals); - Options.ThreadModel = llvm::StringSwitch(CodeGenOpts.ThreadModel) .Case("posix", llvm::ThreadModel::POSIX) Index: cfe/trunk/test/CodeGen/attr-mrecip.c === --- cfe/trunk/test/CodeGen/attr-mrecip.c +++ cfe/trunk/test/CodeGen/attr-mrecip.c @@ -0,0 +1,7 @@ +// RUN: %clang_cc1 -mrecip=!sqrtf,vec-divf:3 -emit-llvm %s -o - | FileCheck %s + +int baz(int a) { return 4; } + +// CHECK: baz{{.*}} #0 +// CHECK: #0 = {{.*}}"reciprocal-estimates"="!sqrtf,vec-divf:3" + Index: cfe/trunk/lib/CodeGen/CGCall.cpp === --- cfe/trunk/lib/CodeGen/CGCall.cpp +++ cfe/trunk/lib/CodeGen/CGCall.cpp @@ -1730,6 +1730,9 @@ FuncAttrs.addAttribute("no-trapping-math", llvm::toStringRef(CodeGenOpts.NoTrappingMath)); + +// TODO: Are these all needed? +// unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags. FuncAttrs.addAttribute("no-infs-fp-math", llvm::toStringRef(CodeGenOpts.NoInfsFPMath)); FuncAttrs.addAttribute("no-nans-fp-math", @@ -1746,6 +1749,12 @@ "correctly-rounded-divide-sqrt-fp-math", llvm::toStringRef(CodeGenOpts.CorrectlyRoundedDivSqrt)); +// TODO: Reciprocal estimate codegen options should apply to instructions? +std::vector = getTarget().getTargetOpts().Reciprocals; +if (!Recips.empty()) + FuncAttrs.addAttribute("reciprocal-estimates", + llvm::join(Recips.begin(), Recips.end(), ",")); + if (CodeGenOpts.StackRealignment) FuncAttrs.addAttribute("stackrealign"); if (CodeGenOpts.Backchain) Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp === --- cfe/trunk/lib/CodeGen/BackendUtil.cpp +++ cfe/trunk/lib/CodeGen/BackendUtil.cpp @@ -533,9 +533,6 @@ llvm::TargetOptions Options; - if (!TargetOpts.Reciprocals.empty()) -Options.Reciprocals = TargetRecip(TargetOpts.Reciprocals); - Options.ThreadModel = llvm::StringSwitch(CodeGenOpts.ThreadModel) .Case("posix", llvm::ThreadModel::POSIX) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel added inline comments. > mehdi_amini wrote in CGCall.cpp:1735 > I think I remember folks being against FMF on calls (Chris Lattner?), I'll > try to find the relevant discussion. > Otherwise your plan seems fine to me! Yes - Chris was opposed to FMF on intrinsics (preferring parameters/metadata as the delivery mechanism instead) in 2012: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20121217/159446.html ...which you mentioned and I replied to in the post-commit thread when I added FMF to any FPMathOperator calls earlier this year: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160104/323154.html There were no replies to that thread on llvm-dev since my January post. I will re-post the question to llvm-dev before proceeding. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
echristo added inline comments. > mehdi_amini wrote in CGCall.cpp:1735 > I think I remember folks being against FMF on calls (Chris Lattner?), I'll > try to find the relevant discussion. > Otherwise your plan seems fine to me! Agreed. Also shouldn't hold up this patch :) https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel added inline comments. > mehdi_amini wrote in CGCall.cpp:1735 > I wonder if we couldn’t have this part of the bitcode/IR auto-upgrade: when > we load a function with this attribute, we automatically add the individual > flag on every instruction. Auto-upgrading is part of the solution. Based on how we've been doing this with vector intrinsics that get converted to IR, it's a ~3-step process: 1. Prepare the backend (DAG) to handle the expected new IR patterns and add tests for those. 2. Auto-upgrade the IR, remove deprecated handling of the old IR patterns, and change/remove existing tests. 3. Update clang to not produce the deprecated patterns. The extra step for FMF in the DAG is that we still don't allow FMF on all SDNode subclasses. The DAG plumbing for FMF only applies to binops because that's all that FMF on IR worked on at the time (fmul/fadd/fsub/fdiv/frem). Later, I added FMF to IR calls so we could have that functionality on sqrt, fma and other calls. Assuming that is ok (and I realize that it may be controversial), we can now extend FMF in the DAG to all SDNodes and have a full node-level FMF solution for the DAG layer. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
mehdi_amini added inline comments. > spatel wrote in CGCall.cpp:1735 > Good point - I think we have to convert all codegen tests that have these > function-level attributes to IR FMF and make sure that the output doesn't > change. Definitely not part of this patch, but hopefully something that can > be done incrementally, test-by-test. I wonder if we couldn’t have this part of the bitcode/IR auto-upgrade: when we load a function with this attribute, we automatically add the individual flag on every instruction. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel added inline comments. > mehdi_amini wrote in CGCall.cpp:1735 > I agree with getting on a path to remove these function attributes that have > an equivalent on per-instruction flag. > > I wonder what is the status of these flags in SelectionDAG though? We still > have a variant of the flags on the TargetOptions I believe. Are all the uses > migrated to per-node flags? Good point - I think we have to convert all codegen tests that have these function-level attributes to IR FMF and make sure that the output doesn't change. Definitely not part of this patch, but hopefully something that can be done incrementally, test-by-test. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
mehdi_amini added inline comments. > echristo wrote in CGCall.cpp:1735 > Would be nice to get these pulled into a single fast-math string that's set > and then used all over for sure. :) I agree with getting on a path to remove these function attributes that have an equivalent on per-instruction flag. I wonder what is the status of these flags in SelectionDAG though? We still have a variant of the flags on the TargetOptions I believe. Are all the uses migrated to per-node flags? https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
echristo added inline comments. > spatel wrote in CGCall.cpp:1735 > I'm probably not imagining some use case, but I was hoping that we can just > delete the 4 (fast/inf/nan/nsz) that are already covered by instruction-level > FMF. An auto-upgrade might be needed within LLVM...and/or a big pile of > regression test changes? No, that seems reasonable. I think it makes more sense as a follow-on patch though. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel updated this revision to Diff 73364. spatel added a comment. Patch updated as suggested by Eric: 1. The attribute is named "reciprocal-estimates". 2. Remove unnecessary -disable-llvm-optzns flag from test file. Quick fixes, but this will not go in until the LLVM side (https://reviews.llvm.org/D24816) is updated, and we've answered any remaining questions there. https://reviews.llvm.org/D24815 Files: lib/CodeGen/BackendUtil.cpp lib/CodeGen/CGCall.cpp test/CodeGen/attr-mrecip.c Index: test/CodeGen/attr-mrecip.c === --- test/CodeGen/attr-mrecip.c +++ test/CodeGen/attr-mrecip.c @@ -0,0 +1,7 @@ +// RUN: %clang_cc1 -mrecip=!sqrtf,vec-divf:3 -emit-llvm %s -o - | FileCheck %s + +int baz(int a) { return 4; } + +// CHECK: baz{{.*}} #0 +// CHECK: #0 = {{.*}}"reciprocal-estimates"="!sqrtf,vec-divf:3" + Index: lib/CodeGen/CGCall.cpp === --- lib/CodeGen/CGCall.cpp +++ lib/CodeGen/CGCall.cpp @@ -1730,6 +1730,9 @@ FuncAttrs.addAttribute("no-trapping-math", llvm::toStringRef(CodeGenOpts.NoTrappingMath)); + +// TODO: Are these all needed? +// unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags. FuncAttrs.addAttribute("no-infs-fp-math", llvm::toStringRef(CodeGenOpts.NoInfsFPMath)); FuncAttrs.addAttribute("no-nans-fp-math", @@ -1746,6 +1749,12 @@ "correctly-rounded-divide-sqrt-fp-math", llvm::toStringRef(CodeGenOpts.CorrectlyRoundedDivSqrt)); +// TODO: Reciprocal estimate codegen options should apply to instructions? +std::vector = getTarget().getTargetOpts().Reciprocals; +if (!Recips.empty()) + FuncAttrs.addAttribute("reciprocal-estimates", + llvm::join(Recips.begin(), Recips.end(), ",")); + if (CodeGenOpts.StackRealignment) FuncAttrs.addAttribute("stackrealign"); if (CodeGenOpts.Backchain) Index: lib/CodeGen/BackendUtil.cpp === --- lib/CodeGen/BackendUtil.cpp +++ lib/CodeGen/BackendUtil.cpp @@ -529,9 +529,6 @@ llvm::TargetOptions Options; - if (!TargetOpts.Reciprocals.empty()) -Options.Reciprocals = TargetRecip(TargetOpts.Reciprocals); - Options.ThreadModel = llvm::StringSwitch(CodeGenOpts.ThreadModel) .Case("posix", llvm::ThreadModel::POSIX) Index: test/CodeGen/attr-mrecip.c === --- test/CodeGen/attr-mrecip.c +++ test/CodeGen/attr-mrecip.c @@ -0,0 +1,7 @@ +// RUN: %clang_cc1 -mrecip=!sqrtf,vec-divf:3 -emit-llvm %s -o - | FileCheck %s + +int baz(int a) { return 4; } + +// CHECK: baz{{.*}} #0 +// CHECK: #0 = {{.*}}"reciprocal-estimates"="!sqrtf,vec-divf:3" + Index: lib/CodeGen/CGCall.cpp === --- lib/CodeGen/CGCall.cpp +++ lib/CodeGen/CGCall.cpp @@ -1730,6 +1730,9 @@ FuncAttrs.addAttribute("no-trapping-math", llvm::toStringRef(CodeGenOpts.NoTrappingMath)); + +// TODO: Are these all needed? +// unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags. FuncAttrs.addAttribute("no-infs-fp-math", llvm::toStringRef(CodeGenOpts.NoInfsFPMath)); FuncAttrs.addAttribute("no-nans-fp-math", @@ -1746,6 +1749,12 @@ "correctly-rounded-divide-sqrt-fp-math", llvm::toStringRef(CodeGenOpts.CorrectlyRoundedDivSqrt)); +// TODO: Reciprocal estimate codegen options should apply to instructions? +std::vector = getTarget().getTargetOpts().Reciprocals; +if (!Recips.empty()) + FuncAttrs.addAttribute("reciprocal-estimates", + llvm::join(Recips.begin(), Recips.end(), ",")); + if (CodeGenOpts.StackRealignment) FuncAttrs.addAttribute("stackrealign"); if (CodeGenOpts.Backchain) Index: lib/CodeGen/BackendUtil.cpp === --- lib/CodeGen/BackendUtil.cpp +++ lib/CodeGen/BackendUtil.cpp @@ -529,9 +529,6 @@ llvm::TargetOptions Options; - if (!TargetOpts.Reciprocals.empty()) -Options.Reciprocals = TargetRecip(TargetOpts.Reciprocals); - Options.ThreadModel = llvm::StringSwitch(CodeGenOpts.ThreadModel) .Case("posix", llvm::ThreadModel::POSIX) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel added inline comments. > echristo wrote in CGCall.cpp:1735 > Would be nice to get these pulled into a single fast-math string that's set > and then used all over for sure. :) I'm probably not imagining some use case, but I was hoping that we can just delete the 4 (fast/inf/nan/nsz) that are already covered by instruction-level FMF. An auto-upgrade might be needed within LLVM...and/or a big pile of regression test changes? https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel marked 2 inline comments as done. spatel added a comment. Thanks, Eric. I actually drafted this with the name "recip-estimates", but thought there might be value in reusing the programmer-visible flag name. I'm good with "reciprocal-estimates" too. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
echristo accepted this revision. echristo added a comment. This revision is now accepted and ready to land. Going to accept this pending the backend patch, but when that one is applied I wanted you to feel OK to add this. A couple of inline nitpick comments and some agreement that we should do something uniform for fast-math is all. -eric > CGCall.cpp:1735 > +// TODO: Are these all needed? > +// unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags. > FuncAttrs.addAttribute("no-infs-fp-math", Would be nice to get these pulled into a single fast-math string that's set and then used all over for sure. :) > CGCall.cpp:1755 > +if (!Recips.empty()) > + FuncAttrs.addAttribute("mrecip", > + llvm::join(Recips.begin(), Recips.end(), ",")); I commented on naming here in the backend patch, but just want to make them the same. > attr-mrecip.c:1 > +// RUN: %clang_cc1 -mrecip=!sqrtf,vec-divf:3 -disable-llvm-optzns -emit-llvm > %s -o - | FileCheck %s > + Shouldn't need to disable the llvm optimizations here. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel added a comment. Ping. https://reviews.llvm.org/D24815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24815: [clang] make reciprocal estimate codegen a function attribute
spatel created this revision. spatel added reviewers: echristo, evandro, hfinkel. spatel added a subscriber: cfe-commits. Herald added subscribers: mehdi_amini, mcrosier. Technically, I suppose this patch is independent of the upcoming llvm sibling patch because we can still pass 'check-all' with this alone. But this patch should be tightly coupled with that patch when committed because there's nothing currently in llvm to read this new function attribute string. The motivation for the change is that we can't have pseudo-global settings for codegen living in TargetOptions because that doesn't work with LTO. And yet, there are so many others there... Ideally, these reciprocal attributes will be moved to the instruction-level via FMF, metadata, or something else. But making them function attributes is at least an improvement over the current mess. https://reviews.llvm.org/D24815 Files: lib/CodeGen/BackendUtil.cpp lib/CodeGen/CGCall.cpp test/CodeGen/attr-mrecip.c Index: test/CodeGen/attr-mrecip.c === --- test/CodeGen/attr-mrecip.c +++ test/CodeGen/attr-mrecip.c @@ -0,0 +1,7 @@ +// RUN: %clang_cc1 -mrecip=!sqrtf,vec-divf:3 -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s + +int baz(int a) { return 4; } + +// CHECK: baz{{.*}} #0 +// CHECK: #0 = {{.*}}"mrecip"="!sqrtf,vec-divf:3" + Index: lib/CodeGen/CGCall.cpp === --- lib/CodeGen/CGCall.cpp +++ lib/CodeGen/CGCall.cpp @@ -1730,6 +1730,9 @@ FuncAttrs.addAttribute("no-trapping-math", llvm::toStringRef(CodeGenOpts.NoTrappingMath)); + +// TODO: Are these all needed? +// unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags. FuncAttrs.addAttribute("no-infs-fp-math", llvm::toStringRef(CodeGenOpts.NoInfsFPMath)); FuncAttrs.addAttribute("no-nans-fp-math", @@ -1746,6 +1749,12 @@ "correctly-rounded-divide-sqrt-fp-math", llvm::toStringRef(CodeGenOpts.CorrectlyRoundedDivSqrt)); +// TODO: Reciprocal estimate codegen options should apply to instructions? +std::vector = getTarget().getTargetOpts().Reciprocals; +if (!Recips.empty()) + FuncAttrs.addAttribute("mrecip", + llvm::join(Recips.begin(), Recips.end(), ",")); + if (CodeGenOpts.StackRealignment) FuncAttrs.addAttribute("stackrealign"); if (CodeGenOpts.Backchain) Index: lib/CodeGen/BackendUtil.cpp === --- lib/CodeGen/BackendUtil.cpp +++ lib/CodeGen/BackendUtil.cpp @@ -529,9 +529,6 @@ llvm::TargetOptions Options; - if (!TargetOpts.Reciprocals.empty()) -Options.Reciprocals = TargetRecip(TargetOpts.Reciprocals); - Options.ThreadModel = llvm::StringSwitch(CodeGenOpts.ThreadModel) .Case("posix", llvm::ThreadModel::POSIX) Index: test/CodeGen/attr-mrecip.c === --- test/CodeGen/attr-mrecip.c +++ test/CodeGen/attr-mrecip.c @@ -0,0 +1,7 @@ +// RUN: %clang_cc1 -mrecip=!sqrtf,vec-divf:3 -disable-llvm-optzns -emit-llvm %s -o - | FileCheck %s + +int baz(int a) { return 4; } + +// CHECK: baz{{.*}} #0 +// CHECK: #0 = {{.*}}"mrecip"="!sqrtf,vec-divf:3" + Index: lib/CodeGen/CGCall.cpp === --- lib/CodeGen/CGCall.cpp +++ lib/CodeGen/CGCall.cpp @@ -1730,6 +1730,9 @@ FuncAttrs.addAttribute("no-trapping-math", llvm::toStringRef(CodeGenOpts.NoTrappingMath)); + +// TODO: Are these all needed? +// unsafe/inf/nan/nsz are handled by instruction-level FastMathFlags. FuncAttrs.addAttribute("no-infs-fp-math", llvm::toStringRef(CodeGenOpts.NoInfsFPMath)); FuncAttrs.addAttribute("no-nans-fp-math", @@ -1746,6 +1749,12 @@ "correctly-rounded-divide-sqrt-fp-math", llvm::toStringRef(CodeGenOpts.CorrectlyRoundedDivSqrt)); +// TODO: Reciprocal estimate codegen options should apply to instructions? +std::vector = getTarget().getTargetOpts().Reciprocals; +if (!Recips.empty()) + FuncAttrs.addAttribute("mrecip", + llvm::join(Recips.begin(), Recips.end(), ",")); + if (CodeGenOpts.StackRealignment) FuncAttrs.addAttribute("stackrealign"); if (CodeGenOpts.Backchain) Index: lib/CodeGen/BackendUtil.cpp === --- lib/CodeGen/BackendUtil.cpp +++ lib/CodeGen/BackendUtil.cpp @@ -529,9 +529,6 @@ llvm::TargetOptions Options; - if (!TargetOpts.Reciprocals.empty()) -Options.Reciprocals = TargetRecip(TargetOpts.Reciprocals); - Options.ThreadModel = llvm::StringSwitch(CodeGenOpts.ThreadModel) .Case("posix", llvm::ThreadModel::POSIX) ___ cfe-commits