[clang] make reciprocal estimate codegen a function attribute

[spatel]

Summary

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.

[spatel]

Ping.

[echristo]

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

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

Would be nice to get these pulled into a single fast-math string that's set and then used all over for sure. :)

1755 ↗

(On Diff #72111)

I commented on naming here in the backend patch, but just want to make them the same.

test/CodeGen/attr-mrecip.c

1 ↗

(On Diff #72111)

Shouldn't need to disable the llvm optimizations here.

[spatel]

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.

[spatel]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

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?

[spatel]

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 (D24816) is updated, and we've answered any remaining questions there.

[echristo]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

No, that seems reasonable. I think it makes more sense as a follow-on patch though.

[mehdi_amini]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

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?

[spatel]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

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.

[mehdi_amini]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

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.

[spatel]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

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.

[mehdi_amini]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

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!

[echristo]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

Agreed. Also shouldn't hold up this patch :)

[spatel]

lib/CodeGen/CGCall.cpp

1735 ↗

(On Diff #72111)

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.

cfe/trunk/lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::CreateTargetMachine(bool MustCreateTM) {
   switch (CodeGenOpts.OptimizationLevel) {
   default: break;
   case 0: OptLevel = CodeGenOpt::None; break;
   case 3: OptLevel = CodeGenOpt::Aggressive; break;
   }
 
   llvm::TargetOptions Options;
 
-  if (!TargetOpts.Reciprocals.empty())
-    Options.Reciprocals = TargetRecip(TargetOpts.Reciprocals);
-
   Options.ThreadModel =
     llvm::StringSwitch<llvm::ThreadModel::Model>(CodeGenOpts.ThreadModel)
       .Case("posix", llvm::ThreadModel::POSIX)
       .Case("single", llvm::ThreadModel::Single);
 
   // Set float ABI type.
   assert((CodeGenOpts.FloatABI == "soft" || CodeGenOpts.FloatABI == "softfp" ||
           CodeGenOpts.FloatABI == "hard" || CodeGenOpts.FloatABI.empty()) &&

cfe/trunk/lib/CodeGen/CGCall.cpp

FuncAttrs.addAttribute("less-precise-fpmad",
                            llvm::toStringRef(CodeGenOpts.LessPreciseFPMAD));
 
     if (!CodeGenOpts.FPDenormalMode.empty())
       FuncAttrs.addAttribute("denormal-fp-math",
                              CodeGenOpts.FPDenormalMode);
 
     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",
                            llvm::toStringRef(CodeGenOpts.NoNaNsFPMath));
     FuncAttrs.addAttribute("unsafe-fp-math",
                            llvm::toStringRef(CodeGenOpts.UnsafeFPMath));
     FuncAttrs.addAttribute("use-soft-float",
                            llvm::toStringRef(CodeGenOpts.SoftFloat));
     FuncAttrs.addAttribute("stack-protector-buffer-size",
                            llvm::utostr(CodeGenOpts.SSPBufferSize));
     FuncAttrs.addAttribute("no-signed-zeros-fp-math",
                            llvm::toStringRef(CodeGenOpts.NoSignedZeros));
     FuncAttrs.addAttribute(
         "correctly-rounded-divide-sqrt-fp-math",
         llvm::toStringRef(CodeGenOpts.CorrectlyRoundedDivSqrt));
 
+    // TODO: Reciprocal estimate codegen options should apply to instructions?
+    std::vector<std::string> &Recips = 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)
       FuncAttrs.addAttribute("backchain");
 
     // Add target-cpu and target-features attributes to functions. If
     // we have a decl for the function and it has a target attribute then
     // parse that and add it to the feature set.

cfe/trunk/test/CodeGen/attr-mrecip.c

+// 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"
+