It'd be nice if a single predicate wouldn't need all_of, but I don't feel strongly about it.

Would it be possible to add a test case for AArch64?

It's a logical addition to the semantics of features. Though at the moment RV is probably the only user of this addition, the eventual patch implementing it should not include the RV specific changes, which would be better in a separate patch.

Shouldn't most instructions have the vtype register in Uses? Shouldn't most FP instructions have fflags in Defs?

Please, prefix the title with [RISCV].

As @lenary said, this must be enabled only for those sub targets that actually fuse these instrs. Also, when using the macro fusion pass, you should also enable the post RA pass. Otherwise, it's less effective.

Going thrice!

It LGTM, but let's wait till tomorrow, when the patch will be 3 business days old, for any input.

Except for a handful of gnats, this is really close to LGTM.

In D69987#1815423, @kito-cheng wrote:

The major issue is should we accept/merge an extension which is not ratified/standardized.

It seems to me that all remarks have already been addressed. Is there anything holding this patch? For it pretty much LGTM.

Overall, it looks nearly good enough. I don't know how the scheduler will behave is interesting cases when there is no ShedRead for each argument in the ins list of the instructions. However, it shouldn't be too difficult to add them in the instruction classes.

Can you please clarify how the existing implementation hurts the scheduler and when fused pairs are then broken?

At first glance, this patch seems sensible, but I'm not sure that D70066 is necessary.

🔔 ¡Ping! 🔔

For the record, the issue reported in PR43617 was fixed by rL374243 and a test case was added by rL374453.

Ping! 🔔

Added the hex FP constants for easier auditing.

@efriedma, you had a point. I now trimmed the precision down to one digit short of seeing a change in the mantissa bits.

Indeed, it seemed to be a coarse check in case the following transformations end up calling pow(). However, the only transformation that calls pow() is when shrinking to powf(), which itself chacks for the availability of this routine. So this patch seems to address this issue the wrong way. It seems that removing this check would be better.

In D68257#1690565, @xbolva00 wrote:

Maybe take std::numbers implementation - llvm::numbers?

Split the changes to the AMDGPU target in a separate patch.

Thank you.

Ping! 🔔

🤦🏻‍♂️