Though semantically equivalent in this case, however I think you should use logical ors here not bitwise. + bool any() { + return UnsafeAlgebra | NoNaNs | NoInfs | NoSignedZeros | + AllowReciprocal; + } Gripe: This pattern is probably super fast and has precedence… but the code is non-obvious: SubclassOptionalData (SubclassOptionalData & ~BitToSet) | (B * BitToSet); This is likely one iota slower.. but it sure is easier to get the intent. B ? SubclassOptionalData |= BitToSet : SubclassOptionalData &= ~BitToSet; Otherwise looks good to me. Joe On Nov 15, 2012, at 5:50 PM, Joe Abbey <joe.abbey at gmail.com> wrote:> Trying to apply patches.. > > What's your base revision? > > Joe > > On Nov 15, 2012, at 5:44 PM, Michael Ilseman <milseman at apple.com> wrote: > >> New patches with review feedback incorporated: >> * Changed single letter flags to short abbreviations ('S' ==> 'nsz') >> * Indentation fixes >> * Comments don't state function names >> >> <0002-Fast-math-flags-added-to-FPMathOperator.patch> >> <0003-Fast-math-interfaces-for-Instructions.patch> >> <0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch> >> <0005-Fast-math-flags-for-the-bitcode.patch> >> <0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch> >> <0007-Fast-math-optimization-fold-multiply-by-zero.patch> >> <0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch> >> >> On Nov 15, 2012, at 10:51 AM, Evan Cheng <evan.cheng at apple.com> wrote: >> >>> >>> On Nov 15, 2012, at 10:51 AM, Michael Ilseman <milseman at apple.com> wrote: >>> >>>> >>>> On Nov 15, 2012, at 10:38 AM, Evan Cheng <evan.cheng at apple.com> wrote: >>>> >>>>> Hi Michael, >>>>> >>>>> The patch looks good in general. But I'm a bit concerned about the textural representation about these flags. 'N', 'I', 'S', 'R', 'A' seem cryptic to me. Does it make sense to expand them a bit 'nnan', 'inf', etc.? They definitely need to be documented. >>>>> >>>> >>>> I think it does make sense to expand them to be more readable. Also, the textual representation doesn't have to precisely follow the internal names. What about: >>>> nnan : no nans >>>> ninf : no infs >>>> nsz : no signed zeros >>>> ar: allow reciprocal >>>> fast : unsafe algebra (and implicitly all the others) >>> >>> These seem reasonable to me. Thanks! >>> >>> Evan >>> >>>> >>>> I'll get started on documentation. >>>> >>>>> Evan >>>>> >>>>> On Nov 15, 2012, at 10:17 AM, Michael Ilseman <milseman at apple.com> wrote: >>>>> >>>>>> Attached are some patches for adding in an IR-level mechanism for representing fast-math flags, as discussed in my prior RFC. Patches include infrastructure, API support, textual and bitcode reader/writer support, example optimization, and test cases. >>>>>> >>>>>> <0002-Fast-math-flags-added-to-FPMathOperator.patch> >>>>>> <0003-Fast-math-interfaces-for-Instructions.patch> >>>>>> <0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch> >>>>>> <0005-Fast-math-flags-for-the-bitcode.patch> >>>>>> <0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch> >>>>>> <0007-Fast-math-optimization-fold-multiply-by-zero.patch> >>>>>> <0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch> >>>>>> >>>>>> _______________________________________________ >>>>>> LLVM Developers mailing list >>>>>> LLVMdev at cs.uiuc.edu llvm.cs.uiuc.edu >>>>>> lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>>>> >>>> >>> >> >> _______________________________________________ >> llvm-commits mailing list >> llvm-commits at cs.uiuc.edu >> lists.cs.uiuc.edu/mailman/listinfo/llvm-commits >-------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/925d0c2f/attachment.html>
On Nov 15, 2012, at 3:23 PM, Joe Abbey <joe.abbey at gmail.com> wrote:> Though semantically equivalent in this case, however I think you should use logical ors here not bitwise. > > + bool any() { > + return UnsafeAlgebra | NoNaNs | NoInfs | NoSignedZeros | > + AllowReciprocal; > + } >Will do.> Gripe: This pattern is probably super fast and has precedence… but the code is non-obvious: > > SubclassOptionalData > (SubclassOptionalData & ~BitToSet) | (B * BitToSet); >This is an existing pattern that's used elsewhere in the file, so I had assumed it was well understood and preferred.> This is likely one iota slower.. but it sure is easier to get the intent. > > B ? SubclassOptionalData |= BitToSet : > SubclassOptionalData &= ~BitToSet; > > Otherwise looks good to me. > > Joe > > On Nov 15, 2012, at 5:50 PM, Joe Abbey <joe.abbey at gmail.com> wrote: > >> Trying to apply patches.. >> >> What's your base revision? >> >> Joe >> >> On Nov 15, 2012, at 5:44 PM, Michael Ilseman <milseman at apple.com> wrote: >> >>> New patches with review feedback incorporated: >>> * Changed single letter flags to short abbreviations ('S' ==> 'nsz') >>> * Indentation fixes >>> * Comments don't state function names >>> >>> <0002-Fast-math-flags-added-to-FPMathOperator.patch> >>> <0003-Fast-math-interfaces-for-Instructions.patch> >>> <0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch> >>> <0005-Fast-math-flags-for-the-bitcode.patch> >>> <0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch> >>> <0007-Fast-math-optimization-fold-multiply-by-zero.patch> >>> <0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch> >>> >>> On Nov 15, 2012, at 10:51 AM, Evan Cheng <evan.cheng at apple.com> wrote: >>> >>>> >>>> On Nov 15, 2012, at 10:51 AM, Michael Ilseman <milseman at apple.com> wrote: >>>> >>>>> >>>>> On Nov 15, 2012, at 10:38 AM, Evan Cheng <evan.cheng at apple.com> wrote: >>>>> >>>>>> Hi Michael, >>>>>> >>>>>> The patch looks good in general. But I'm a bit concerned about the textural representation about these flags. 'N', 'I', 'S', 'R', 'A' seem cryptic to me. Does it make sense to expand them a bit 'nnan', 'inf', etc.? They definitely need to be documented. >>>>>> >>>>> >>>>> I think it does make sense to expand them to be more readable. Also, the textual representation doesn't have to precisely follow the internal names. What about: >>>>> nnan : no nans >>>>> ninf : no infs >>>>> nsz : no signed zeros >>>>> ar: allow reciprocal >>>>> fast : unsafe algebra (and implicitly all the others) >>>> >>>> These seem reasonable to me. Thanks! >>>> >>>> Evan >>>> >>>>> >>>>> I'll get started on documentation. >>>>> >>>>>> Evan >>>>>> >>>>>> On Nov 15, 2012, at 10:17 AM, Michael Ilseman <milseman at apple.com> wrote: >>>>>> >>>>>>> Attached are some patches for adding in an IR-level mechanism for representing fast-math flags, as discussed in my prior RFC. Patches include infrastructure, API support, textual and bitcode reader/writer support, example optimization, and test cases. >>>>>>> >>>>>>> <0002-Fast-math-flags-added-to-FPMathOperator.patch> >>>>>>> <0003-Fast-math-interfaces-for-Instructions.patch> >>>>>>> <0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch> >>>>>>> <0005-Fast-math-flags-for-the-bitcode.patch> >>>>>>> <0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch> >>>>>>> <0007-Fast-math-optimization-fold-multiply-by-zero.patch> >>>>>>> <0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> LLVM Developers mailing list >>>>>>> LLVMdev at cs.uiuc.edu llvm.cs.uiuc.edu >>>>>>> lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>>>>> >>>>> >>>> >>> >>> _______________________________________________ >>> llvm-commits mailing list >>> llvm-commits at cs.uiuc.edu >>> lists.cs.uiuc.edu/mailman/listinfo/llvm-commits >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/90a8a604/attachment.html>
Another round of improved patches, and a patch for documentation changes to LangRef. * Make comments more up to date * Use 'arcp' instead of 'ar' * Use logical || Still based off of r168110 On Nov 15, 2012, at 3:31 PM, Michael Ilseman <milseman at apple.com> wrote:> > On Nov 15, 2012, at 3:23 PM, Joe Abbey <joe.abbey at gmail.com> wrote: > >> Though semantically equivalent in this case, however I think you should use logical ors here not bitwise. >> >> + bool any() { >> + return UnsafeAlgebra | NoNaNs | NoInfs | NoSignedZeros | >> + AllowReciprocal; >> + } >> > > Will do. > >> Gripe: This pattern is probably super fast and has precedence… but the code is non-obvious: >> >> SubclassOptionalData >> (SubclassOptionalData & ~BitToSet) | (B * BitToSet); >> > > This is an existing pattern that's used elsewhere in the file, so I had assumed it was well understood and preferred. > >> This is likely one iota slower.. but it sure is easier to get the intent. >> >> B ? SubclassOptionalData |= BitToSet : >> SubclassOptionalData &= ~BitToSet; >> >> Otherwise looks good to me. >> >> Joe >> >> On Nov 15, 2012, at 5:50 PM, Joe Abbey <joe.abbey at gmail.com> wrote: >> >>> Trying to apply patches.. >>> >>> What's your base revision? >>> >>> Joe >>> >>> On Nov 15, 2012, at 5:44 PM, Michael Ilseman <milseman at apple.com> wrote: >>> >>>> New patches with review feedback incorporated: >>>> * Changed single letter flags to short abbreviations ('S' ==> 'nsz') >>>> * Indentation fixes >>>> * Comments don't state function names >>>> >>>> <0002-Fast-math-flags-added-to-FPMathOperator.patch> >>>> <0003-Fast-math-interfaces-for-Instructions.patch> >>>> <0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch> >>>> <0005-Fast-math-flags-for-the-bitcode.patch> >>>> <0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch> >>>> <0007-Fast-math-optimization-fold-multiply-by-zero.patch> >>>> <0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch> >>>> >>>> On Nov 15, 2012, at 10:51 AM, Evan Cheng <evan.cheng at apple.com> wrote: >>>> >>>>> >>>>> On Nov 15, 2012, at 10:51 AM, Michael Ilseman <milseman at apple.com> wrote: >>>>> >>>>>> >>>>>> On Nov 15, 2012, at 10:38 AM, Evan Cheng <evan.cheng at apple.com> wrote: >>>>>> >>>>>>> Hi Michael, >>>>>>> >>>>>>> The patch looks good in general. But I'm a bit concerned about the textural representation about these flags. 'N', 'I', 'S', 'R', 'A' seem cryptic to me. Does it make sense to expand them a bit 'nnan', 'inf', etc.? They definitely need to be documented. >>>>>>> >>>>>> >>>>>> I think it does make sense to expand them to be more readable. Also, the textual representation doesn't have to precisely follow the internal names. What about: >>>>>> nnan : no nans >>>>>> ninf : no infs >>>>>> nsz : no signed zeros >>>>>> ar: allow reciprocal >>>>>> fast : unsafe algebra (and implicitly all the others) >>>>> >>>>> These seem reasonable to me. Thanks! >>>>> >>>>> Evan >>>>> >>>>>> >>>>>> I'll get started on documentation. >>>>>> >>>>>>> Evan >>>>>>> >>>>>>> On Nov 15, 2012, at 10:17 AM, Michael Ilseman <milseman at apple.com> wrote: >>>>>>> >>>>>>>> Attached are some patches for adding in an IR-level mechanism for representing fast-math flags, as discussed in my prior RFC. Patches include infrastructure, API support, textual and bitcode reader/writer support, example optimization, and test cases. >>>>>>>> >>>>>>>> <0002-Fast-math-flags-added-to-FPMathOperator.patch> >>>>>>>> <0003-Fast-math-interfaces-for-Instructions.patch> >>>>>>>> <0004-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch> >>>>>>>> <0005-Fast-math-flags-for-the-bitcode.patch> >>>>>>>> <0006-Fast-math-test-case-for-bitcode-and-textual-reading-.patch> >>>>>>>> <0007-Fast-math-optimization-fold-multiply-by-zero.patch> >>>>>>>> <0008-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch> >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> LLVM Developers mailing list >>>>>>>> LLVMdev at cs.uiuc.edu llvm.cs.uiuc.edu >>>>>>>> lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>>>>>> >>>>>> >>>>> >>>> >>>> _______________________________________________ >>>> llvm-commits mailing list >>>> llvm-commits at cs.uiuc.edu >>>> lists.cs.uiuc.edu/mailman/listinfo/llvm-commits >>> >> > > _______________________________________________ > llvm-commits mailing list > llvm-commits at cs.uiuc.edu > lists.cs.uiuc.edu/mailman/listinfo/llvm-commits-------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Fast-math-flags-added-to-FPMathOperator.patch Type: application/octet-stream Size: 4660 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-Fast-math-interfaces-for-Instructions.patch Type: application/octet-stream Size: 7400 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0001.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0002.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0003-Fast-math-flags-for-LLVM-IR-parsing-and-printing.patch Type: application/octet-stream Size: 5117 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0002.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0003.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0004-Fast-math-flags-for-the-bitcode.patch Type: application/octet-stream Size: 3192 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0003.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0004.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0005-Fast-math-test-case-for-bitcode-and-textual-reading-.patch Type: application/octet-stream Size: 5646 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0004.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0005.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0006-Fast-math-optimization-fold-multiply-by-zero.patch Type: application/octet-stream Size: 4361 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0005.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0006.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0007-Fast-math-test-for-SimplifyInstruction-fold-multiply.patch Type: application/octet-stream Size: 1498 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0006.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0007.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0008-Fast-math-flags-documentation-added-to-LangRef.patch Type: application/octet-stream Size: 8678 bytes Desc: not available URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0007.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <lists.llvm.org/pipermail/llvm-dev/attachments/20121115/c3115159/attachment-0008.html>