In D85366#2211716, @jrtc27 wrote:

In D85366#2211651, @evandro wrote:

hasSideEffects may imply isNotDuplicable, especially when rematerializing, but the latter prevents duplication more extensively in the middle end (e.g., tail end duplication).

But why does that matter? If LLVM decides it's better to fork a code path then let it, whether it's a CSR instruction or not really does not matter, it's just I-cache usage. In fact that's another reason why I _wouldn't_ want this change to go through.

hasSideEffects may imply isNotDuplicable, especially when rematerializing, but the latter prevents duplication more extensively in the middle end (e.g., tail end duplication).
Indeed, isReMaterializable (default to false) applies here too.

I'm not familiar with the AMDGPU target, but the changes in these new tests seem harmless, except in CodeGen/AMDGPU/stack-realign.ll, where an instruction disappeared. Again, I'd defer to someone who works in this backend, perhaps @arsenm, to chime in.

Other than that, it seems sensible.

The AMDGPU test change likely means nothing, but it'd be good if someone who maintains or work that target would ok it. I suggest giving it another week or so. Otherwise, it LGTM.

Just a couple of nits, but otherwise it LGTM.

It LGTM after D80802.

It LGTM, but it's better to wait for an OK from a couple or so of other reviewers to chime in.

Again, the clang part should be split in another patch and be made a child of D81188.

It looks pretty GTM. At this point, I'd be fine with accepting this patch as the major issues seem to have already been addressed. Should there be any other minor issue, it could be addressed later.

In D78129#2023772, @joelkevinjones wrote:

list<Predicate> UnsupportedFeatures = !listconcat(SVEUnsupported.F, PAUnsupported.F);

In D79409#2023674, @foad wrote:

No, the sign of log2(x) depends on whether x is less or greater than 1, not less or greater than 2.

pow(x, -∞) → ∞ if |x| < 1 or 0 if |x| > 1
pow(x, +∞) → 0 if |x| < 1 or ∞ if |x| > 1
exp2(log2(x) * -∞) → exp2(±∞) → ∞ if x < 2 or 0 if x > 2
exp2(log2(x) * +∞) → exp2(±∞) → 0 if x < 2 or ∞ if x > 2

pow(∞, y) → 0 if y < 0 or ∞ if y > 0
exp2(log2(∞) * y) → exp2(∞ * y) → exp2(∞) → 0 if y < 0 or ∞ if y > 0

LGTM

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.