- User Since
- Jul 21 2020, 2:19 PM (140 w, 3 d)
Thu, Mar 30
I think this can catch bugs earlier, LGTM.
Wed, Mar 29
SGPR_HI16 is already marked as not allocatable. So L611 was partially redundant with that. But this patch does appear to remove a barrier to allocating into the hi or lo half of TMA_LO or some other non-GPR SReg.
I guess there is other code somewhere preventing allocating to those non-GPR SReg? If so, this patch can be marked NFC and the commit reworded to say it skips redundant reservations.
Wed, Mar 22
Fri, Mar 10
This looks like it fixes 49830, and I agree with foad it is useful in the AMDGPU backend. Functionally LGTM! Thanks for working on it.
Feb 22 2023
I'm fine with the patch, but it's a shame the solution isn't more robust.
Its not self-evident why runline is written like that, so I expect uses without the ':' could creep back in.
Feb 21 2023
Feb 20 2023
- There is a bug in permlane16 selection I am working on and will put up a patch for soon
- I'm curious how AMDGPU/permlane-op-sel.ll test passes. When I run the runline "llc -march=amdgcn -mcpu=gfx1030 -show-mc-encoding" using ToT and check the output I get "v_permlane16_b32 v0, v0, s7, s0 op_sel:[1,0] ; encoding: [0x00,0x49,0x77,0xd7,0x00,0x0f,0x00,0x20]" not what is in the check line.
Thanks for taking a look at this issue.
Feb 10 2023
LGTM besides my previously noted typo, but please wait for @arsenm
Feb 3 2023
Feb 1 2023
Jan 31 2023
Jan 26 2023
Overall I like the patch. It definitely improves readability.
Jan 6 2023
Jan 5 2023
Further cleanups are possible, such as deleting one of these fields completely or cleaning up AsmVOP3OpSel, but I'd prefer to do it incrementally.
Jan 4 2023
Dec 30 2022
The code looks fine, but as you say, the change visible in user code and could break something. Do you want to handle that somehow? Maybe wait for @b-sumner
Dec 29 2022
LGTM! Thanks for catching that.
Dec 14 2022
Dec 13 2022
The last functional status I see here is.
Dec 12 2022
Dec 7 2022
precommit gfx11 runline and rebase
Dec 6 2022
Dec 5 2022
Overall this is looking quite good.
do this on an "opt-in" basis
Makes sense to me
Dec 2 2022
Dec 1 2022
Ok, apart from that comment LGTM.
Nov 30 2022
Overall this looks pretty good. As you say, the feature checking logic is quite limited, but that's not a problem.
I think after this patch lands https://reviews.llvm.org/D123693 can be reverted. Can you try to revert that with this patch and check if device libs can be built correctly at -O0?
Nov 29 2022
Nov 28 2022
Its not just SGPRs we want to disable, right? Its inline and literal operands too. So the commit title is slightly misleading.
Nov 23 2022
Thanks to Jay as well for pointing out std::optional on another review.
I believe the latest guidance is to use std::Optional instead of llvm::Optional (https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716). Also it makes a difference in tests, so is not NFC. Otherwise looks good.
Nov 21 2022
Nov 18 2022
Nov 17 2022
This is a creative use of the opName, but I think it probably hinders readability. No other instructions are doing this, so when I look at definition of the Real instruction it seems to refer to the non-strict pseudo.
Nov 16 2022
See my comment, otherwise I like it.
Nov 15 2022
Nov 14 2022
This looks like a reasonable refactor. Why does it change the error position?
Nov 11 2022
Nov 10 2022
Nov 3 2022
This part of the commit message is worded confusingly for me.
Nov 2 2022
Oct 25 2022
use const auto &, hoist loop invariant code, shorten td line length, delete parameter name underscores
Oct 24 2022
Oct 21 2022
Oct 20 2022
Overall looks good. Did you observe any instructions that assembled when they shouldn't? It looks like only differences in error message from the tests.
Oct 19 2022