[PATCH] D155730: [PowerPC] Add a pass to merge all of the constant globals into one pool.

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:49
+    // issue with incorrect alignent that would require padding.
+    Align LHSAlign = LHS->getAlign().valueOrOne();
+    Align RHSAlign = RHS->getAlign().valueOrOne();
----------------
amyk wrote:
> Do we need to check size and alignment together?
> 
At this point I've added a limitation where where the alignment cannot be larger than the size of the element in order to be included in the pool. This means that at this point it is sufficient to just check the alignment here.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:78
+  }
+} CompareConstants;
+
----------------
amyk wrote:
> New group code review questions:
> - Have we considered alignment based string pool struct?
> - Have we considered making this target independent and making a Discourse discussion regarding this?
> 
I think that both of these are good suggestions.

I think that one struct per alignment may be a good idea but I think it should be controlled by an option and added as a later patch. 

This pass does not rely on anything that is PowerPC specific and so it could be turned into a target independent pass. However, I would prefer to try this for PowerPC first because I go ahead and try to make it target independent for a number of targets. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:90
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addPreserved<DominatorTreeWrapperPass>();
+    AU.addPreserved<LoopInfoWrapperPass>();
----------------
amyk wrote:
> Question: I think we discussed this before when talking about constant merging, but how do we determine which passes to preserve again? And how does this different from the constant pool merge patch?
When I look at passes to preserve I generally add this pass and then look to see which analysis passes are added after this one in the pipeline. After that I check the passes that are added to see if it makes sense to preserve them. It's not really a hard science and it is a bit of a judgement call because I have to have an understanding of what each pass does.

This pass is in a very different place in the pipeline compared to the constant pool patch. This pass is before instruction selection and so it is an IR pass and not a machine IR pass. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:111
+
+bool PPCMergeStringPool::runOnModule(Module &M) {
+  bool Changed = false;
----------------
amyk wrote:
> Is there a reason why this is declared outside, rather than inside the class?
Moved inside the class.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:113
+  bool Changed = false;
+  Context = &M.getContext();
+
----------------
amyk wrote:
> Can we get the context later on when we need it?
Moved this out.


================
Comment at: llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp:147
+      continue;
+
+    if (Global.hasSection())
----------------
amyk wrote:
> - Can we document why the global cannot have a section and metadata?
> - Can we combine these into `||`? 
I'm not going to merge these two into one `if` statement. They have nothing to do with each other and I think it's more clear if they are separate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155730/new/

https://reviews.llvm.org/D155730