- User Since
- Jan 25 2017, 7:29 AM (155 w, 5 d)
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.
Sep 4 2019
Jul 29 2019
Jul 26 2019
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?
Jul 17 2019
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.
Jul 15 2019
Jun 21 2019
May 9 2019
Apr 23 2019
Mar 20 2019
Mar 18 2019
Mar 12 2019
Mar 11 2019
Mar 7 2019
Modified test in line with review comments
Mar 6 2019
Mar 5 2019
Mar 4 2019
Feb 11 2019
Not really an area I'm 100% sure about - but looks ok to me. One of the other reviewers will have to sign off too.
Minor niggle on the comment (if my understanding is correct).
Feb 4 2019
Jan 14 2019
Dec 11 2018
LGTM - but probably need approval from one of the other reviewers as well
Nov 29 2018
Nov 28 2018
Nov 27 2018
Nov 19 2018
Thanks for the review - made all the suggested changes