Page MenuHomePhabricator

[AIX] Static init frontend recovery and backend support
ClosedPublic

Authored by Xiangling_L on Jul 24 2020, 8:35 AM.

Details

Summary
  1. Frontend side
  2. Recovered AIX static init frontend to use the linkage type and function names Clang chooses for sinit related function;
  3. Removed the GlobalUniqueModuleId calculation and usage;
  4. Adjusted the FE testcases accordingly;
  5. Added one frontend testcase to demonstrate and validate separate initialization on AIX;
  1. Backend side on the assembly path only
  2. Set correct linkage and function names for sinit/sterm functions
  3. Added testcases

Event Timeline

Xiangling_L created this revision.Jul 24 2020, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2020, 8:35 AM

Fix clang-tidy errors;

jasonliu added inline comments.Jul 27 2020, 8:07 AM
clang/lib/CodeGen/ItaniumCXXABI.cpp
4609

Handling template instantiation seems fairly orthogonal to "moving naming logic from frontend to backend". Could we put it in a separate patch (which could be a child of this one)?

llvm/include/llvm/CodeGen/AsmPrinter.h
486

Remove const from value type (e.g. Priority and index), as there is no real need for it.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
2159

Maybe a naive quesiton, in what situation would we have name collision and need index as suffix to differentiate?
Could we just report_fatal_error in those situation?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1885

This would have conflict with D84363. You might want to rebase it later.

1955

Changing Function name and linkage underneath looks a bit scary. People would have a hard time to map IR to final assembly that gets produced. Have you thought about creating an alias (with the correct linkage and name) to the original function instead?

Xiangling_L marked 5 inline comments as done.Jul 28 2020, 7:05 AM
Xiangling_L added inline comments.
clang/lib/CodeGen/ItaniumCXXABI.cpp
4609

The reason I chose to handle template instantiation and inline variable in this patch is that I want to show the scenarios where we have separate initialization. And this is also the reason why we want to embed array index into sinit/sterm functions. That is, if we move this part into a separate patch, I am worried that ppl will feel it's weird to embed index into function names.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
2159

As far as I know, there are several situations we would have separate initialization without considering priority:

  1. template specialization
  2. inline variable
  3. ctor/dtor attribute functions

By embedding the index, we can group those sinit/sterm with same priority within current file together.

And as I mentioned above, I chose to handle the former two cases in this patch to show why we need to embed the index.

And regarding the third scenario, we've already report_fatal_error on those cases.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1885

Thanks for the reminder, I will update the patch accordingly.

1955

Good point. I will adjust the implementation here.

Xiangling_L marked 4 inline comments as done.

Created alias for sinit and sterm;
Adjusted the testcase accordingly;

Removed the disablement in IncrementalProcessingTest.cpp cross-target test;

jasonliu added inline comments.Jul 30 2020, 12:42 PM
clang/lib/CodeGen/CGDeclCXX.cpp
23–24

We are removing the usage of "getUniqueModuleId" in this file, So I assume this include could get removed as well.

clang/lib/CodeGen/CodeGenModule.h
1058

This wrapper seems redundant. Calling AddGlobalDtor(StermFinalizer, Priority); directly from the callee side already convey what we are trying to achieve here.

clang/lib/CodeGen/ItaniumCXXABI.cpp
4602

Could we trigger this error? If so, could we have a test for it?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2119 ↗(On Diff #284355)

I think it's still useful to keep these functions around to prevent accidentally calling to these functions on AIX. We could rephrase error message to "static constructor/destructor section is not needed on AIX."

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1875

Have some suggestion on structure backend code change:

  1. Remove isSpecialLLVMGlobalArrayForStaticInit and isSpecialLLVMGlobalArrayToSkip, and call emitSpecialLLVMGlobal directly instead. This would handle all the llvm. variable for us. We would need do early return for names start with llvm. in emitGlobalVariable as well, since they are all handled here.
  1. We could override emitXXStructorList because how XCOFF handle the list is vastly different than the other target. We could make the common part(i.e. processing and sorting) a utility function, say "preprocessStructorList".
  1. It's not necessary to use the same name emitXXStructor if the interface is different. It might not provide much help when we kinda know no other target is going to use this interface. So we could turn it into a lambda inside of the overrided emitXXStructorList, or simply no need for a new function because the logic is fairly straightforward.
jasonliu added inline comments.Jul 30 2020, 12:50 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1865

We will need to move this part of the if statement to the overrided emitXXStructorList as well. (If we think overriding emitXXStructorList and calling emitSpecialLLVMGlobal directly is a good idea.)

Xiangling_L marked 6 inline comments as done.Aug 4 2020, 9:22 AM
Xiangling_L added inline comments.
clang/lib/CodeGen/CodeGenModule.h
1058

I added this wrapper function to keep the symmetry between AddGlobalCtor and AddGlobalDtor. They are private functions within CodeGenModule class. And I feel it's kinda weird to only move AddGlobalDtor to a public function.

clang/lib/CodeGen/ItaniumCXXABI.cpp
4602

I should've put an assertion here. The init priority attribute has been disabled on AIX in the previous patch.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1875
  1. As we discussed offline, we would leave the handling of llvm.used and llvm.metadata later and don't include them in the scope of this patch.
Xiangling_L marked 3 inline comments as done.Aug 4 2020, 9:23 AM
Xiangling_L added inline comments.Aug 4 2020, 12:52 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1865

getUniqueModuleId takes Module as a parameter, if we want to invoke this function inside of emitXXStructorList through emitSpecialLLVMGlobal, we have to change the interface of them, which seems not ideal.

Refactor the code by making emitXXStructorList a virtual function;
Added one testcase for .ref;
Addressed other comments;

jasonliu added inline comments.Aug 5 2020, 7:44 AM
format
1 ↗(On Diff #283006)

Redundant file?

llvm/include/llvm/CodeGen/AsmPrinter.h
481

The new block is not all Overridable Hooks, seems better belong in Coarse grained IR lowering routines.

482

This struct got moved to header, means it's not an implementation detail any more.
We would need doxygen comment on it.

486

I have a slight preference to leave it as ComdatKey, and change the name when we actually need to handle .ref because without seeing the actual implementation for .ref we don't know if this is the way we want to pursue.

492

A doxygen comment describe what this function does, and what its return value means, and mention Structors is an output argument.
By looking at what this function does, it seems buildStructorList is a better name.

494

Add a blank line above this.

llvm/test/CodeGen/PowerPC/aix-static-init-key-object.ll
7

Adding this test case would looks like we already decided how to handle .ref in clang and llvm. But in fact, we haven't. I would prefer not having this test.

Xiangling_L marked 7 inline comments as done.Aug 5 2020, 2:13 PM
Xiangling_L added inline comments.
llvm/include/llvm/CodeGen/AsmPrinter.h
492

I meant to and should've named this function to preprocessXXStructorList, let me know if you still prefer buildStructorList. And if you do, since the underneath of SmallVector is a variable-sized array, maybe we should try buildSortedStructorArray?

llvm/test/CodeGen/PowerPC/aix-static-init-key-object.ll
7

As we discussed offline, I would adjust the error message instead.

Xiangling_L marked 2 inline comments as done.

Added descriptions for struct and functions;
Addressed other comments;

jasonliu added inline comments.Aug 6 2020, 7:24 AM
llvm/include/llvm/CodeGen/AsmPrinter.h
391

This description is not entirely true. We only see if the array is a ConstantArray and returning false. We are not returning false if the array's element is not { i32, void ()*, i8* }.

492

preprocess sounds like we are already having a XXStructorList and now we try to do something on it.
But right now, we are actually passing in an empty StructorList/Array and build it from scratch. So I would still prefer the name of build in it.
I don't mind changing to a more accurate name as you suggested.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
2103

Return of boolean seems unnecessary.
Callee could check the size of the Structors to decide if they want an early return or not (or in this particular case, the for loop would just do nothing and no need for extra condition if you don't mind the call to getPointerPrefAlignment or assign to 0 to Index)?
So we could just return void for this function?

2126

nit: Missing blank line.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1966

nit: Use ++Index instead.
https://llvm.org/docs/CodingStandards.html#prefer-preincrement

Or use Index++ at line 1963 so that we don't need this line.

Xiangling_L marked 5 inline comments as done.Aug 6 2020, 12:04 PM
Xiangling_L added inline comments.
llvm/include/llvm/CodeGen/AsmPrinter.h
492

I think we do have a XXStructorList here which is the initializer of llvm.gloal_ctorsor llvm.gloal_dtors array? The usage of this term is consistent with other spots.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
2103

Yeah, we could do that. But it looks a boolean return value makes the code flow natural. And if any target does want to control the early return in the future, it's flexbile for them to do that. I am wondering is there any big difference between bool and void return value for this function?

jasonliu added inline comments.Aug 6 2020, 12:20 PM
llvm/include/llvm/CodeGen/AsmPrinter.h
492

My understanding is that before we enter this preprocessXXStructorList, we do not have a list of XXStructor. We only have a list of Constant. This functions turns a list of Constant to a list of XXStructor.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
2103

If target need to control early return in the future, they could still do so by querying if the output Structors is empty or not.
I just don't want unnecessary returns, as it's one more thing that user of the function need to care about when they examine this function. And the user of this function would have the same question of why we need to return a boolean here.

Xiangling_L marked 3 inline comments as done.Aug 6 2020, 1:07 PM
Xiangling_L added inline comments.
llvm/include/llvm/CodeGen/AsmPrinter.h
492

Just leave a record here, as we discussed offline, we agree that the meaning of term XXStructorList is the initializer of llvm.gloal_ctors` or llvm.gloal_dtors array. So I will keep using preprocessXXStructorList as the function name.

Xiangling_L marked an inline comment as done.
jasonliu accepted this revision.Aug 7 2020, 6:46 AM

Thanks. LGTM.

This revision is now accepted and ready to land.Aug 7 2020, 6:46 AM