- User Since
- Jan 25 2017, 7:29 AM (220 w, 6 d)
You might want to wait for Valery to comment too.
Thu, Apr 8
Windows builds give a warning without this change
Tue, Apr 6
Mar 15 2021
Mar 9 2021
This looks good to me - @critson has done a pre-emptive merge/test with this patch on our internal branch for graphics and it looks good.
Jan 21 2021
This LGTM - but I agree with your comment that this should probably not be a separate pass.
Jan 14 2021
Jan 13 2021
More in line with other similar fixes
Jan 11 2021
Nov 17 2020
Sep 23 2020
This change looks good to me, especially now that you've made the requested changes.
Sep 21 2020
Sep 18 2020
See comment about a VC++ warning being generated.
Aug 14 2020
LGTM (minor comment typo), but I'm adding Tim who did most of the original code I think.
Jul 10 2020
D81662 started causing warnings for VS. This fixes those.
Jun 24 2020
Made requested changes
Jun 8 2020
I'm thinking that this fix may be taking the wrong approach now.
Jun 3 2020
Rebase to get tests to pass
Improved the test to include one that has a call provoking s32 to be initialized
Jun 2 2020
Apr 2 2020
My fix has been hanging around for a little while now - so my memory of it is a little hazy.
Mar 31 2020
Mar 19 2020
Tested this in my environment and it fixed the issue.
Mar 17 2020
Added more reviewers - anyone got an opinion on this one.
Mar 6 2020
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.
Mar 5 2020
Dec 17 2019
Confirmed that this fix also solves my original problem per D70595
Dec 10 2019
Alternative fix D70975 works.
This fixes the issue I encountered for the original fix in D70595
Dec 4 2019
Dec 3 2019
Made suggested changes
Dec 2 2019
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).
Nov 28 2019
Nov 27 2019
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.
Nov 26 2019
Matt - are you happy for this to proceed?
Added test updates for failing lit tests.
Nov 22 2019
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)
Nov 21 2019
Oct 25 2019
Maybe put [AMDGPU] into the subject line of the commit message
Oct 16 2019
Oct 7 2019
Sep 30 2019
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).
Sep 23 2019
Made suggested changes - is this more in line with what you were thinking Matt?
Updates in light of review comments
Sep 19 2019
Made some changes based on review
Sep 18 2019
GFX10 support has gone in since this change was approved - gfx10 allows 2 sgprs
on the constant bus. Implementation updated to allow for this.