Skip to content
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

Move the PowerPC/PPCMergeStringPool work to initializer #77352

Merged

Conversation

scui-ibm
Copy link
Contributor

@scui-ibm scui-ibm commented Jan 8, 2024

Currently, the PPCMergeStringPool merges the global variable after the AsmPrinter initializer adds the global variables to its symbol list. This is to move the merging work of PPCMergeStringPool to its initializer, just like what GlobalMerge does, to avoid adding merged global variables to the AsmPrinter symbol lis.  

@scui-ibm
Copy link
Contributor Author

gentle ping - any comments/approval?

Copy link
Contributor

@stefanp-ibm stefanp-ibm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that on AIX in order to have the correct alignment for a particular csect we need to have the global defined in the Initialization phase instead of the Run phase. Is this the issue that we are trying to solve here? Or, am I missing another issue with deleting Globals after Initialization?

Is it not possible to fix this for the AIX AsmPrinter instead of having to force all of the passes to make all of their changes to Globals in the Initialization phase? I feel like going forward there may be problems especially with Target Independent passes.

@@ -1148,7 +1148,7 @@ entry:

attributes #0 = { nounwind }

; AIXDATA: .csect L..__ModuleStringPool[RO],2
; AIXDATA: .csect L..__ModuleStringPool[RO],3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment change for ModuleStringPool csect. I assume that this is the problem you are trying to fix.

@scui-ibm
Copy link
Contributor Author

I understand that on AIX in order to have the correct alignment for a particular csect we need to have the global defined in the Initialization phase instead of the Run phase. Is this the issue that we are trying to solve here? Or, am I missing another issue with deleting Globals after Initialization?

Thanks Stefan for the comments. Actually, the code change is to fix one of our customer problems. The problem is when AsmPrinter pass run is trying to find the csect of a symbol, the compiler emits an error as the symbol is already merged/removed by PPCMergeStringPool.  The change of #77364 is for the same customer problem, and with that change the compiler error still occurs. As the symbol merged is already added to the symbol table during doInitilization of AsmPrinter (but before PPCMergeStringPool).  We can also fix this during AsmPrinter run, but I believe it’s cleaner with current change – we don’t need to worry about the uses of symbol table of the AsmPrinter for the removed symbols.

Is it not possible to fix this for the AIX AsmPrinter instead of having to force all of the passes to make all of their changes to Globals in the Initialization phase? I feel like going forward there may be problems especially with Target Independent passes.

I’m not that familiar with all the llc passes, but I checked all the doInitilization function of those passes and I couldn’t think of potential issues with this change based on my understanding. Do you have any idea of potential problems with this change?

@stefanp-ibm
Copy link
Contributor

I understand that on AIX in order to have the correct alignment for a particular csect we need to have the global defined in the Initialization phase instead of the Run phase. Is this the issue that we are trying to solve here? Or, am I missing another issue with deleting Globals after Initialization?

Thanks Stefan for the comments. Actually, the code change is to fix one of our customer problems. The problem is when AsmPrinter pass run is trying to find the csect of a symbol, the compiler emits an error as the symbol is already merged/removed by PPCMergeStringPool. The change of #77364 is for the same customer problem, and with that change the compiler error still occurs. As the symbol merged is already added to the symbol table during doInitilization of AsmPrinter (but before PPCMergeStringPool). We can also fix this during AsmPrinter run, but I believe it’s cleaner with current change – we don’t need to worry about the uses of symbol table of the AsmPrinter for the removed symbols.

Is it not possible to fix this for the AIX AsmPrinter instead of having to force all of the passes to make all of their changes to Globals in the Initialization phase? I feel like going forward there may be problems especially with Target Independent passes.

I’m not that familiar with all the llc passes, but I checked all the doInitilization function of those passes and I couldn’t think of potential issues with this change based on my understanding. Do you have any idea of potential problems with this change?

Okay, that's fair. I'm also not aware of existing problems with this. I'm not actually concerned about this patch as much as the design that requires us to move passes that modify globals into the init stage. I'm not going to block this patch.

Copy link
Contributor

@stefanp-ibm stefanp-ibm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@scui-ibm scui-ibm merged commit 1bab570 into llvm:main Jan 31, 2024
4 of 5 checks passed
@scui-ibm scui-ibm deleted the merge_string_pool_during_initialization branch January 31, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants