User Details
- User Since
- Dec 17 2012, 10:03 AM (497 w, 2 d)
Yesterday
I looked and don't see anything for this. We fold all of these in separate MI passes
Tue, Jun 28
I always found it very unfortunate that we keep IR references around in MIR to do alias analysis queries. This appears to remove a lot of those users (all but the ones in the schedule graph construction?), so I highly welcome the change!
I'm not sure what context this gets called in. Maybe it's possible to hit this for DPP instructions?
Fri, Jun 24
Do you know if AMDGPU has any convergent instruction involved in pattern with folding?
I could use that to create a test case.
Fri, Jun 10
Wed, Jun 1
Nice catch!
But that's not a problem of this patch, and we will address that later if needed.
May 12 2022
May 11 2022
@fhahn are you okay with the clang tests update as well?
May 6 2022
Hi Jay,
May 2 2022
Thanks for the heads-up and the revert.
- includes fixes to clang tests that were missed in the original commit.
Alright, at first I thought it was a latent bug in the dead argument elimination pass and now I think the two failing tests just need updating.
I'll upload a diff with the fix tests. (See below for the diff of the tests themselves.)
Ah thanks for the heads-up, looking at the clang issue.
I see what you are trying to solve, but at the same time, in theory any instruction could have this kind of register pressure problems. (Though, we probably don't ever create too many not spillable live-ranges around regular instructions and regular instructions have a reasonable number of operands.)
Apr 29 2022
Apr 18 2022
Apr 13 2022
Nice catch!
The verifier doesn't like that at all since it doesn't have an associated class. Plus every target would have to handle encoding it as something in the end
The patch itself looks good but I wonder if we could maybe take a simpler road:
Could we assign register zero (noreg) to this failed vreg?
Apr 7 2022
Apr 1 2022
At first glance it looks similar to what @aditya_nandakumar implemented internally to get rid of copies produced by eviction chains.
Mar 29 2022
Thanks @arsenm for tracking this down and the fix!
Mar 25 2022
Hmm, that's weird, if one register fails within the same level, we should roll back the whole level to its previous state.
I now have another attempt which tracks a stack of successful recolorings that are rolled back on failure
I'm wondering if a better strategy would be to try avoiding recolor candidates that could conflict with the saved recoloring state for fixed registers
Mar 18 2022
Hi Jonas,
The complicated cases are indeed not well tested since it is very hard to hit them. I was hoping Matt's fix would avoid falling in one of them, while not preventing actual success. Looks like it wasn't the case.
Feb 21 2022
I haven't looked into it yet, but we see cases of "ran out of registers during register allocation" with this patch, for code that compiled succesfully before.
Feb 16 2022
The fix looks good to me.
Feb 15 2022
Jan 18 2022
Jan 14 2022
Jan 13 2022
Jan 10 2022
Jan 5 2022
- Set the default smallvector size to 10
Jan 4 2022
Dec 23 2021
Nov 30 2021
LGTM, nits below
Nov 29 2021
Nov 16 2021
Hi Mircea,
Nov 15 2021
should we go ahead and revert (most of*) https://reviews.llvm.org/D35816?
Nov 2 2021
I'm also guessing that you'd want to avoid using string keys for the function-attribute implementation?
Let me just step back a little bit and say that now that I think about what we did, having something that answers "should I run in this instance" is desirable, the implementation doesn't really matter. We did it with function attributes, but having a bisect client API like you're introducing is fine.
My only complain is that the client interface should not have remote in the name :P.
Hi Amara,
Hi Amara,
Oct 27 2021
Oct 26 2021
Oct 1 2021
I don't know if it's safe to rely on the kill flag being on the first operand that uses a particular reg. If not I'll rework the patch.
This seems accidental. I think ideally we would enforce consistency of the flag within an instruction. Short of that we better not assume anything like just the first operand having a kill flag...
Sep 27 2021
Makes sense, thanks!
Sep 24 2021
This mostly seems to benefit X86
Sep 17 2021
One last comment, otherwise LGTM.
Sep 16 2021
Sep 14 2021
Aug 31 2021
LGTM
Aug 12 2021
Looks good to me as well.
Jul 28 2021
Given that, can you approve Philip's patch?
Following the conversation in https://reviews.llvm.org/D105723#2894209, approving this patch.
Jul 21 2021
Hi all,
LGTM
Jul 13 2021
I've tried approaches with target pseudo intrinsics and extra IR passes, but still everything depends on missing the external (relatively to the IRTranslator) way to obtain the info about Value->Vreg mapping.
I am not sure I understand the issue here.
Could you paste the IR with the wrong transformation (before this patch) for one of the test (e.g., the smaller one)?
Jul 7 2021
Jun 29 2021
I'm suspicious of how simple this patch is, and not sure why this special phi handling is needed given that refineSubRanges was already called
Looks fine to me but please wait on the comment from Jay (@foad ) and Matt (@arsenm ) before pushing.
Nitpick below.
Jun 17 2021
Jun 16 2021
Hi Amara,
Jun 8 2021
Hi Nick,
Jun 4 2021
By removing the reportTranslationError() temporarily in order to test my implementation, that would cause errors in other targets. Can you please suggest me any other way to solve this issue?
Jun 3 2021
LGTM
May 25 2021
LLVM ERROR: unable to translate in big endian mode (in function: f)
May 24 2021
Add GlobalISel infrastructure up to the point where we can select a ret void.
May 19 2021
I wouldn't be so sure about that. One particular usecase I need this for is a bitcast between pointer types:
%1 = bitcast i8* %0 to %struct.ST*
In our target they're both of p0 LLTs, so the default method eliminates this instruction while in our target it's necessary to keep it.
May 12 2021
LGTM.
Disclaimer: I didn't really look at the AMDGPU changes.