[clang] [llvm] Reimplement constrained 'trunc' using operand bundles (PR #118253)

2025-01-09 Thread Kevin P. Neal via cfe-commits

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)

2025-01-08 Thread Kevin P. Neal via cfe-commits

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)

2025-01-08 Thread Kevin P. Neal via cfe-commits


@@ -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)

2024-12-16 Thread Serge Pavlov via cfe-commits


@@ -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)

2024-12-13 Thread Kevin P. Neal via cfe-commits


@@ -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)

2024-12-08 Thread Matt Arsenault via cfe-commits

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)

2024-12-06 Thread Eli Friedman via cfe-commits

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)

2024-12-06 Thread Matt Arsenault via cfe-commits

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)

2024-12-05 Thread Matt Arsenault via cfe-commits

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)

2024-12-05 Thread Eli Friedman via cfe-commits

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)

2024-12-05 Thread Kevin P. Neal via cfe-commits


@@ -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)

2024-12-01 Thread via cfe-commits

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)

2024-12-01 Thread Serge Pavlov via cfe-commits

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