LukeZhuang marked 12 inline comments as done.
LukeZhuang added a comment.
In D79830#2075038 <https://reviews.llvm.org/D79830#2075038>, @davidxl wrote:
> Can you add some tests at the LLVM side?
added ll test case
================
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)
----------------
LukeZhuang wrote:
> 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.
Sorry, after I test with different targets I understood your suggestion. It
shows weird behavior in comparing for AVR target. Now I have converted to
IEEEdouble. I also added test for AVR target and passed it.
================
Comment at: clang/test/Sema/builtin-expect-with-probability.cpp:18
+ bool dummy;
+ dummy = __builtin_expect_with_probability(x > 0, 1, 0.9);
+ dummy = __builtin_expect_with_probability(x > 0, 1, 1.1); // expected-error
{{probability of __builtin_expect_with_probability is outside the range [0.0,
1.0]}}
----------------
rsmith wrote:
> Please also test some more edge cases. such as: if the probability argument
> is not convertible to float, or is inf or nan, negative zero, -denorm_min, or
> 1+epsilon.
added more edge cases here and example for AVR target
================
Comment at: llvm/docs/BranchWeightMetadata.rst:18
Branch weights might be fetch from the profiling file, or generated based on
-`__builtin_expect`_ instruction.
+`__builtin_expect`_ and `__builtin_expect_with_probability`_ instruction.
----------------
rsmith wrote:
> There are no instructions with these names. There are Clang builtin functions
> with these names and LLVM intrinsics with similar but different names. This
> document should be documenting the `llvm.expect` and
> `llvm.expect.with.probability` intrinsics.
I also think I might had better add intrinsic documents in
llvm/docs/LangRef.rst instead of here, and keep
__builtin_exepct_with_probability here or discuss with the original author
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
