-
Notifications
You must be signed in to change notification settings - Fork 15.6k
[CFIInserter] Improve CSRSavedLocation struct.
#168869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) MLIRMLIR.Dialect/XeGPU/propagate-layout-subgroup.mlir (Likely Already Failing)This test is already failing at the base commit.If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
|
I pulled out a subset of this into #170721 with the goal of getting some of the API changes in, and then returning to the enum/union bits in this change. |
(1) Define `CSRSavedLocation::Kind` and use it in the code. This makes the code more readable and allows to extend it to new kinds. For example, soon I want to add "scalable offset from a given register" kind. (2) Store the contents in a union. This should reduce memory usage.
c1ffa81 to
51edcce
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
preames
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point to an example where the Kind will simplify code? On it's own, I'm not a fan of remaining parts of this change. I tried to dig through your other open reviews, but didn't find the motivating usage.
| CSRLoc = CSRSavedLocation::createRegister(*CSRReg); | ||
| else if (CSROffset) | ||
| CSRLoc = CSRSavedLocation::createCFAOffset(*CSROffset); | ||
| if (CSRLoc.isValid()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we not add the Invalid state. The prior code didn't need it, your switch based code shouldn't either. Is there a strong reason this needs to exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be needed here: https://github.com/llvm/llvm-project/pull/168531/files#diff-54d9d06f60ce6c927ea0f2c1380a50bdf93c689d1781186966ef2234660e47c9R421
Unless we want to collect all callee-saved registers in a vector before we come to this line. But... we also need CFIs for exception handling (or something else) and I suspect it may require emitting CFIs for non-callee saved registers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: I don't think we'll ever need to care about CFIs for registers which are not returned by getCalleeSavedRegs, so we can make CFIInstrInserter only track those. That should save some memory too. I'll create a separate PR for that.
|
The point of this is to prepare for future work where
|
🪟 Windows x64 Test Results
Failed Tests(click on a test name to see its output) MLIRMLIR.Dialect/XeGPU/propagate-layout-subgroup.mlirIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
|
The CFIInstrInserter pass in LLVM can insert |
|
@MaskRay Thanks for taking a look!
Yes. From the comments in
I think we can also implement this optimization in But since this is an optimization, I think we can postpone it until all the core functionality works. I vaguely remember that I measured the size of cfi sections and it was significantly larger, so it will be important. P.S. actually now when I think about it, it is unclear if the algorithm in EDIT: if a block is it's own predecessor then CFI state on entry and exit to this block have to be the same. So that's not a good example. |
|
Consider the following example: after PEI, CFIs will be generated: Here we see that two different CFI states for When we need to initalize the incoming CFI state for @preames I hope this justifies why we need the |
(1) Define
CSRSavedLocation::Kindand use it in the code. This makes the code more readable and allows to extend it to new kinds. For example, soon I want to add "scalable offset from a given register" kind.(2) Store the contents in a union. This should reduce memory usage.