LukeZhuang added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1818-1819
+ llvm::APFloat Probability = Eval.Val.getFloat();
+ if (!(Probability >= llvm::APFloat(0.0) &&
+ Probability <= llvm::APFloat(1.0))) {
+ Diag(ProbArg->getLocStart(), diag::err_probability_out_of_range)
----------------
rsmith wrote:
> I think this will probably only work if the target `double` type is
> `IEEEdouble`. (Otherwise you'll be comparing an `IEEEdouble` `APFloat` to a
> different kind of `APFloat`.)
>
> We do not guarantee that `double` is `IEEEdouble` for all our supported
> targets. In particular, for TCE and for AVR (at least under `-mdouble=32`),
> `double` is `IEEEsingle` instead.
>
> It looks like your LLVM intrinsic expects an LLVM IR `double` (that is, an
> `IEEEdouble`) as its third argument, so perhaps the right thing to do would
> be to convert the `APFloat` to `IEEEdouble` here and in CodeGen. Please add a
> test for a target where `double` is not `IEEEdouble`.
Hi, thank you for comments! I have several questions here:
(1) I do not understand why "APFloat Probability >= APFloat(0.0)" is affected
by target here. Is this just a double come from C front-end and I check it
here?
(2) And also not sure in CGBuiltin, is there I convert it to llvm intrinsic?
How does target double type affect it?
(3) If I convert APFloat to IEEEDouble, should I use APF.convertToDouble or
create a DoubleAPFloat. My original code did APFloat.convertToDouble and
compare, and I do not sure about the difference.
================
Comment at: llvm/docs/BranchWeightMetadata.rst:128-135
+Built-in ``expect.with.probability`` Instruction
+================================================
+
+``__builtin_expect_with_probability(long exp, long c, double probability)`` has
+the same semantics as ``__builtin_expect``, but the caller provides the
+probability that ``exp == c``. The last argument ``probability`` must be
+constant floating-point expression and be in the range [0.0, 1.0] inclusive.
----------------
rsmith wrote:
> This whole section of documentation (line 86 to 166) seems inappropriate to
> me as LLVM documentation, because it's talking about the behavior of one
> particular source language with one particular frontend, not LLVM semantics.
> Here's what I'd suggest:
>
> * Move all of that documentation into the Clang user's manual, as a
> description of the `__builtin_expect*` builtin functions.
> * Here in the LLVM documentation, add documentation for the `llvm.expect`
> and `llvm.expect.with.probability` intrinsic functions, and include a note
> pointing to the Clang documentation and suggesting that `__builtin_expect`
> and `__builtin_expect_with_probability` can be used to generate these
> intrinsics in C and C++ source files.
Yeah, thank you! I think your suggestion makes much sense, it should be in
clang. However, the original __builtin_expect part is not written by me and and
I am not sure its affect. So I added the author of this part as reviewer and I
think it had better to ask his idea about this.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79830/new/
https://reviews.llvm.org/D79830
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
