This change looks good to me, especially now that you've made the requested changes.

See comment about a VC++ warning being generated.

LGTM (minor comment typo), but I'm adding Tim who did most of the original code I think.

D81662 started causing warnings for VS. This fixes those.

Made requested changes

I'm thinking that this fix may be taking the wrong approach now.

Rebase to get tests to pass

Improved the test to include one that has a call provoking s32 to be initialized

My fix has been hanging around for a little while now - so my memory of it is a little hazy.

In D75741#1926383, @tstellar wrote:

How would it invalidate the list? Isn't the analysis of which instructions can be moved done one list at a time?

Tested this in my environment and it fixed the issue.

Added more reviewers - anyone got an opinion on this one.

Formatting fixes

I encountered this issue with a test shader I was looking at. I did manage to create a reproducer, but a different fix to the triviallyDisjoint function meant that it stopped happening.
However, I still think the situation may occur, hence putting this change up for review.

Confirmed that this fix also solves my original problem per D70595

Alternative fix D70975 works.

This fixes the issue I encountered for the original fix in D70595

In D70595#1775820, @lebedev.ri wrote:

@dstuttard does D70975 cover the original problem motivating this patch?

Made suggested changes

I would have preferred to have put this check into isImmOperandLegal in SIInstrInfo.cpp - but that produced lots of lit regressions. Looks like commute operations use this function even when they are swapping rather than replacing the operand (which breaks).

Formatting

Changed the implementation slightly.
Now the isNegatibleForFree includes a bool for allowing multi-use
constants. This is only set to true for calls from getNegatedExpression so it
only allows for the situation where a constant becomes multi-use during
getNegatedExpression recursive evaluation.

Matt - are you happy for this to proceed?

Added test updates for failing lit tests.

In D70595#1756702, @RKSimon wrote:

I'd like to see more of the regressions on other targets (x86 in particular) - in many cases this will cause a lot of extra constant pool entries.

I've got some failures as a result of this change in X86 (haven't tried the other backends). I guess that the failures are probably due to the fneg propagation working better for constants - but I haven't investigated yet. There will need to be more test changes as a result of this change.
My question is - does the change look reasonable? If so I'll take a look at the lit regressions for other backends.

Included wrong version of the test initially

Added @RKSimon to review as the last major modifier of the code here.

Replace COUNT-n with n DAG versions (DAG-COUNT-n doesn't work)

Maybe put [AMDGPU] into the subject line of the commit message

LGTM

ping

Matt - are you now happy for me to submit this? (It is tagged as approved, but since you've made some extra comments I'm waiting for you to agree with the latest changes).

Made suggested changes - is this more in line with what you were thinking Matt?

Updates in light of review comments

Made some changes based on review

GFX10 support has gone in since this change was approved - gfx10 allows 2 sgprs
on the constant bus. Implementation updated to allow for this.

Managed to get the fmac test to keep using fmac
Also updated the test to use non-anonymous values

Changed test to use fma intrinsic

+Stas to comment on the v_fmac_f16 test change.
Is it acceptable to change the result to look for v_pk_fma_f16 rather than 2 v_fmac_f16 instructions? If not, any suggestions on how to get the compiler to generate 2 x fmac instead?

I might revisit this one - setting cumode seems messy to enable driver control of the WGP setting, but seems the most pragmatic at the moment.

ping