[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
kpneal wrote: D146845 wasn't committed because it would have caused test failures. The tests are wrong, the new checks reveal this, but the new checks cannot be committed until all broken tests are fixed. Otherwise we get failures from the bots and the Verifier checks would have been reverted. The D146845 ticket encodes the current rules for the strictfp attribute. If you are making changes that fail with D146845 applied to your tree then you are moving in the wrong direction. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
kpneal wrote: I do think that before we start adding in code like this ticket we need to add IR Verifier code to check for proper use of the strictfp attribute. This code never made it into the tree because there are too many broken tests already in the tree. Verifier code could be written that only fires when an error is detected AND no constrained intrinsics are used in a function. This should eliminate failures from most, but not all, of the currently broken tests. Hopefully the few broken tests that are in tree and fire will be small enough that they can be fixed. The remainder of the broken tests will be corrected over time or will simply be removed. My ticket that never got pushed is here: https://reviews.llvm.org/D146845 I can provide a current version of that code if it would be useful. I also have checks that are implemented on top of that code to ensure that regular FP instructions are never mixed with constrained intrinsics. We'll need to push something like that hopefully not long after we start putting this bundle support into the tree. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
@@ -86,6 +86,43 @@ IRBuilderBase::createCallHelper(Function *Callee, ArrayRef Ops, return CI; } +CallInst *IRBuilderBase::CreateCall(FunctionType *FTy, Value *Callee, +ArrayRef Args, +ArrayRef OpBundles, +const Twine &Name, MDNode *FPMathTag) { + ArrayRef ActualBundlesRef = OpBundles; + SmallVector ActualBundles; + + if (IsFPConstrained) { +if (const auto *Func = dyn_cast(Callee)) { + if (Intrinsic::ID ID = Func->getIntrinsicID()) { +if (IntrinsicInst::canAccessFPEnvironment(ID)) { + bool NeedRound = true, NeedExcept = true; + for (const auto &Item : OpBundles) { +if (NeedRound && Item.getTag() == "fpe.round") + NeedRound = false; +else if (NeedExcept && Item.getTag() == "fpe.except") + NeedExcept = false; +ActualBundles.push_back(Item); kpneal wrote: I think #1 is a better choice despite the downsides. Having more opportunities to optimize when lowering if we know the current rounding mode seems like a good choice. It does simplify the implementation in places as you said. Having the rounding mode bundle specify the FP environment is a change from the constrained intrinsics, and this point is a little fine, so I do think we'll need to clearly state this in the LangRef at some point. I am a little worried that we're creating a footgun and someone may write code that relies on the rounding mode bundle when handling trunc, floor, or one of the other math intrinsics/library calls. Then again, if code is trying to evaluate an expression then a switch is going to be needed with entries that would be expected to have rounding modes hardcoded into them. So I'm not worried enough to change my view that #1 is preferred. Having the rounding mode bundle specify the FP environment also means we don't need any Verifier checks for improperly present or specified rounding bundles. That's a minor win. One last point: this is another example of how having the rounding mode specified is useful. Since we've defined the constrained intrinsics to require the rounding mode be correct, and an incorrect rounding mode is undefined behavior, we can rely on the specified rounding mode being correct. The constant folding that we're doing currently checks the rounding mode in the cases I remember. We should carry over in the LangRef the verbiage about incorrect rounding mode metadata being UB. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
@@ -86,6 +86,43 @@ IRBuilderBase::createCallHelper(Function *Callee, ArrayRef Ops, return CI; } +CallInst *IRBuilderBase::CreateCall(FunctionType *FTy, Value *Callee, +ArrayRef Args, +ArrayRef OpBundles, +const Twine &Name, MDNode *FPMathTag) { + ArrayRef ActualBundlesRef = OpBundles; + SmallVector ActualBundles; + + if (IsFPConstrained) { +if (const auto *Func = dyn_cast(Callee)) { + if (Intrinsic::ID ID = Func->getIntrinsicID()) { +if (IntrinsicInst::canAccessFPEnvironment(ID)) { + bool NeedRound = true, NeedExcept = true; + for (const auto &Item : OpBundles) { +if (NeedRound && Item.getTag() == "fpe.round") + NeedRound = false; +else if (NeedExcept && Item.getTag() == "fpe.except") + NeedExcept = false; +ActualBundles.push_back(Item); spavloff wrote: It depend on how we want to treat the rounding mode bundle. At least two cases are possible. (1) The rounding mode bundle specifies the floating-point environment. That is it provides information about the current value of the rounding mode in FPCR. If optimizer can deduce this value, it may set the appropriate value in all affected instruction. For example, in the following code: ``` call @llvm.set_rounding(i32 1) %v = float call @llvm.trunc(float %x) ``` the call to `trunc` can be replaced with: ``` %v = float call @llvm.trunc(float %x) [ "fpe.control"(metadata !"rte") ] ``` The rounding mode in this bundle does not change the meaning of `trunc`, but could be useful in some cases. The two calls: ``` %v = float call @llvm.trunc(float %x) [ "fpe.control"(metadata !"rte") ] %v = float call @llvm.trunc(float %x) [ "fpe.control"(metadata !"rtz") ] ``` represent the same operation, but on the target where `trunc` is implemented as `round using current mode` the latter instruction is implemented as one operation, while the former generally requires three operations (`set fpcr`, `nearbyint`, `set fpcr`). This is a hypothetical example however. It seems the meaning of current rounding metadata argument in the constrained intrinsics agrees with this model, see discussion in https://discourse.llvm.org/t/static-rounding-mode-in-ir/80621. In this scenario it does not make much sense to exclude unused rounding mode from allowed bundles. The bundles can be set by optimizer in a simple way, without checking if the instruction uses rounding mode. We use a similar method in clang AST, where all relevant nodes have complete `FPOptions`. (2) The rounding mode bundle specifies the rounding mode used for evaluating the instruction. Instructions like `trunc` do not depend on the specified rounding mode, so it does not make sense to use rounding bundles for them. This viewpoint seems more natural since rounding is considered as a parameter of an operation, similar to arguments. It also can be naturally extended to static FP control modes. Rounding as a parameter produces exactly the same effect no matter if it is read from FPCR or specified in the instruction. Other FP options, such as denormal behavior, can be handled similarly. Neither method has a clear-cut advantage, and we need to discuss which approach to take. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
@@ -86,6 +86,43 @@ IRBuilderBase::createCallHelper(Function *Callee, ArrayRef Ops, return CI; } +CallInst *IRBuilderBase::CreateCall(FunctionType *FTy, Value *Callee, +ArrayRef Args, +ArrayRef OpBundles, +const Twine &Name, MDNode *FPMathTag) { + ArrayRef ActualBundlesRef = OpBundles; + SmallVector ActualBundles; + + if (IsFPConstrained) { +if (const auto *Func = dyn_cast(Callee)) { + if (Intrinsic::ID ID = Func->getIntrinsicID()) { +if (IntrinsicInst::canAccessFPEnvironment(ID)) { + bool NeedRound = true, NeedExcept = true; + for (const auto &Item : OpBundles) { +if (NeedRound && Item.getTag() == "fpe.round") + NeedRound = false; +else if (NeedExcept && Item.getTag() == "fpe.except") + NeedExcept = false; +ActualBundles.push_back(Item); kpneal wrote: Do we want intrinsics where the rounding mode is baked in (like trunc or floor) to be allowed to have a rounding mode bundle? Or, do we want the rounding mode bundle to be able to specify a rounding mode that isn't the one baked into the intrinsic? I'm leaning towards the rounding mode bundle not being allowed. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
arsenm wrote: > Two, we can add a "readnone_fp_intrinsic" attribute, which would mean the > intrinsic is readnone unless there's an operand bundle indicating otherwise. I think this needs to be more refined to FP mode read/write and errno read/write. Basically a mirror of memory() for arguments/other memory https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
efriedma-quic wrote: > > llvm.trunc is currently marked IntrNoMem in Intrinsics.td; you'll need to > > update that if you want it to read/modify FP state. (Trying to override the > > default by sticking attributes on top doesn't work properly, as far as I > > know.) > This this the key point in this solution, - we want to use the same intrinsic > both in default and non-default environment. All properties necessary for > non-default case will be attached to the call site. If something prevents > this plan, we should evaluate it. IntrNoMem gets translated to readnone, i.e. does not access any memory, including FP state. If the intrinsic can in fact read/modify FP state in some cases, we have to remove that from the intrinsic. There are basically two ways we can go from there. One, we can just make the frontend and/or transforms add a readnone marking to callsites that can't actually access FP state (i.e. calls in non-strictfp functions). Two, we can add a "readnone_fp_intrinsic" attribute, which would mean the intrinsic is readnone unless there's an operand bundle indicating otherwise. I think the first way composes more cleanly with our general approach to memory effects. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
arsenm wrote: > If you mean an attribute of a call site, then yes, we need more detailed view > on side effects in non-default environment. Anyway, performance of a program > running with non-default rounding mode should not drop if exception tracking > is not needed. As for attribute of an intrinsic, its purpose seems unclear. I mean a general attribute that can apply to the declaration, and a call site. We need to be able to mark which intrinisic declarations do not care about errno or other fp mode bits, and whether they can read or write them. Furthermore, it is useful to mark individual callsites with stricter variants, just like for memory attributes. We need this to avoid stripping IntrNoMem, it should still be IntrNoMem, with the additional qualifier that strictfp may read/write errno/rounding mode. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
arsenm wrote: > llvm.trunc is currently marked IntrNoMem in Intrinsics.td; you'll need to > update that if you want it to read/modify FP state. (Trying to override the > default by sticking attributes on top doesn't work properly, as far as I > know.) I think we need a dedicated fp env attribute to model this https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
efriedma-quic wrote: llvm.trunc is currently marked IntrNoMem in Intrinsics.td; you'll need to update that if you want it to read/modify FP state. (Trying to override the default by sticking attributes on top doesn't work properly, as far as I know.) https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
@@ -251,10 +251,12 @@ static bool markTails(Function &F, OptimizationRemarkEmitter *ORE) { // Special-case operand bundles "clang.arc.attachedcall", "ptrauth", and // "kcfi". kpneal wrote: This comment needs to be updated. I suggest removing the list that is present in the code and replacing it with text explaining why these operand bundles are special cases. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
https://github.com/graphite-app[bot] edited https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)
spavloff wrote: ### Merge activity * **Dec 2, 12:55 AM EST**: The merge label 'FP Bundles' was detected. This PR will be added to the [Graphite merge queue](https://app.graphite.dev/merges?org=llvm&repo=llvm-project) once it meets the requirements. https://github.com/llvm/llvm-project/pull/118253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits