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
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
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 :)
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.
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
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
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
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
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
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)
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
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
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
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'
14 matches
Mail list logo
