[TableGen] Fix computeRegUnitLaneMasks for ad hoc aliasing #79835
Conversation
With ad hoc aliasing (using the Aliases field in register definitions) we can have subregisters with disjoint lane masks that nevertheless share a regunit. In this case we need to take the union of their lane masks, not the intersection. This was inadvertently broken by https://reviews.llvm.org/D157864
|
Hi @jayfoad , thank you for this PR. I prepared a test for this PR like you asked me in #79642. What is the best way for you to merge the test I made? 1) merge this PR as usual, and I submit another PR for a test? 2) cherry-pick my test to this PR? Either is fine. Please let me know when you have time. Thanks! The test is available at https://github.com/kaz7/llvm-project/commit/b8d3ad4f9ece967ae0512b974893af95575d1af9. |
This test is based on VE implementation. VE has two subregisters. Both are disjointed. However, many instructions contaminate other register very often. Therefore, VE defines them as aliased to avoid register allocator to allocate both registers simultaneously. This trick conflicts wht recent computeRegUnitLaneMasks optimization. The optimization is reverted now. And I'm adding test case for that.
I cherry-picked your test, thanks. But I am no longer sure this is the best approach for fixing the problem. How about #79831 as an alternative? |
|
Hi @jayfoad , Is it possible to ask review others about this PR in order to merge this? As I said at #79831 (comment), I think #79831 is not enough to cover the problem. Recently the modification on live range (#88892) broke VE backend. I don't inspect the patch deeply, but the patch itself seems reasonable. And, if I apply this patch, it solves the problem. I think that it just comes naturally to treat a lanemask == None as empty. So, I'd like to fix the problem from the source. |
Sure, you can ask opinion from others, but personally I am not convinced that this is the best way to fix it.
Do you have a test case for the new breakage? I'd like to take a look. |
|
@jayfoad , don't you think each regunit corresponds to a distinct laneBitmask in a register. Now, in case of ad-hoc aliases, a shared(fake) regunit is created for both the leaf register (as in case of sx0 , sw0 & sf0). Now, if we have unique leaf regunit for each of these 2 leaf regs( as suggested by you #79831 (comment):), then how RA will understand that both sw0 & sf0 are same (as regunits are used for interference check) thus losing the very reason of why ad-hoc-alias is used in first place |
My suggestion is that SW0 and SF0 should each get a regunit that uniquely identifies them as well as a shared regunit that represents the aliasing. For example: SW0 regunits: 0, 1 // regunit 0 is for the aliasing |
Just a thought, even after that we would have invalid lanemask(0x0) for regUnit0, but for remaining regUnit1 & regUnit2, they will rightfully show the lanemask as calculated as per SubRegIdx in SX0 ! But, that's fine I guess right. |
Yes I think that's strictly better than what we have now. |
|
Just want to enlighten on RootRegs of regUnit, they are nothing but the leaf registers itself, and in case of ad hoc aliasing (assuming that in the rarest scenario maximmum cliques in ad hoc graph wouldn't exced 2), so there the other RootReg is just the other alias Reg & vice versa for the regUnit of aliasReg in context. The example of SX0 & its subregs makes it quite evident! |
With ad hoc aliasing (using the Aliases field in register definitions) we can have subregisters with disjoint lane masks that nevertheless share a regunit. In this case we need to take the union of their lane masks, not the intersection.
This was inadvertently broken by https://reviews.llvm.org/D157864