[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG073cc29e04b7: [X86][Reduce] Preserve fast math flags when change

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei updated this revision to Diff 485016. pengfei added a comment. Add fmul, fmin and fmax cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140467/new/ https://reviews.llvm.org/D140467 Files: clang/lib/CodeGen/CGBuiltin.cpp

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

arsenm added inline comments. Comment at: clang/test/CodeGen/builtins-x86-reduce.c:8 +} + +// CHECK: fadd pengfei wrote: > arsenm wrote: > > Should test the builtins from both sets > Do you mean this? Almost. You added the guard to 4 switch cases, so I would

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei added inline comments. Comment at: clang/test/CodeGen/builtins-x86-reduce.c:8 +} + +// CHECK: fadd arsenm wrote: > Should test the builtins from both sets Do you mean this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei updated this revision to Diff 484838. pengfei marked 2 inline comments as done. pengfei added a comment. Address review comments. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140467/new/ https://reviews.llvm.org/D140467 Files:

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

arsenm added a comment. In D140467#4013107 , @pengfei wrote: > Add test case to check FastMathFlagGuard works. > > Not to mention above cases. So it doesn't sound feasible to me. Testing is always feasible. You could even just generate all the

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei updated this revision to Diff 484826. pengfei added a comment. Add test case to check FastMathFlagGuard works. > Tests don't exist for users, they exist for compiler developers... > I agree with @arsenm. At least for clang irgen, we should have good test > coverage. You are right.

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

arsenm added a comment. In D140467#4010675 , @pengfei wrote: > As I have explained, users are not suggested to use these builtins given we > have provided the more stable, well documented corresponding intrinsics. The > only case user has to use it is

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

lebedev.ri requested changes to this revision. lebedev.ri added a comment. In D140467#4010675 , @pengfei wrote: >> If it exists it must be tested. >> Every piece of code generation needs to be tested. > > Let me show you the number: > > $ grep -rho

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei added a comment. > If it exists it must be tested. > Every piece of code generation needs to be tested. Let me show you the number: $ grep -rho '__builtin_ia32\w\+' clang/test/CodeGen | sort|uniq |wc -l 337 $ grep -rho '_mm512_\w\+' clang/test/CodeGen | sort|uniq |wc -l 2304

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

arsenm added a comment. In D140467#4010507 , @arsenm wrote: > In D140467#4010378 , @pengfei wrote: > >> Use FastMathFlagGuard instead, thanks @foad! >> >> In D140467#4010296

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

arsenm requested changes to this revision. arsenm added a comment. This revision now requires changes to proceed. In D140467#4010378 , @pengfei wrote: > Use FastMathFlagGuard instead, thanks @foad! > > In D140467#4010296

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14742 Builder.getFastMathFlags().setAllowReassoc(); -return Builder.CreateCall(F, {Ops[0], Ops[1]}); +Value *FAdd = Builder.CreateCall(F, {Ops[0], Ops[1]}); +Builder.getFastMathFlags() &=

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei updated this revision to Diff 484521. pengfei marked an inline comment as done. pengfei added a comment. Use FastMathFlagGuard instead, thanks @foad! In D140467#4010296 , @arsenm wrote: > Needs tests. I couldn’t find any for the base builtins

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

arsenm added a comment. Needs tests. I couldn’t find any for the base builtins either Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140467/new/ https://reviews.llvm.org/D140467 ___ cfe-commits mailing

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

foad added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14740 CGM.getIntrinsic(Intrinsic::vector_reduce_fadd, Ops[1]->getType()); +FastMathFlags FMF = Builder.getFastMathFlags(); Builder.getFastMathFlags().setAllowReassoc(); We

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

barannikov88 added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14742 Builder.getFastMathFlags().setAllowReassoc(); -return Builder.CreateCall(F, {Ops[0], Ops[1]}); +Value *FAdd = Builder.CreateCall(F, {Ops[0], Ops[1]}); +

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

barannikov88 added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:14742 Builder.getFastMathFlags().setAllowReassoc(); -return Builder.CreateCall(F, {Ops[0], Ops[1]}); +Value *FAdd = Builder.CreateCall(F, {Ops[0], Ops[1]}); +

[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI

pengfei created this revision. pengfei added reviewers: RKSimon, arsenm. Herald added a project: All. pengfei requested review of this revision. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. @arsenm raised a good question that we should use a flag guard. But I found