<div dir="ltr">Hi Rafael,<div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-05 22:01 GMT+08:00 Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On 5 June 2014 03:26, Jiangning Liu <<a href="mailto:liujiangning1@gmail.com" target="_blank">liujiangning1@gmail.com</a>> wrote:<br>
> Hi Rafael,<br>
><br>
> I think now I understand your point and agree with you this alignment setup<br>
> can be treated as a pure back-end issue AArch64 can handle it privately and<br>
> we can avoid exposing it to shared code.<br>
><br>
> So now attached is the updated patch by removing -global-merge-aligned<br>
> switch. I will work out separate patches to handle AArch64 specific stuff<br>
> later on.<br>
<br>
</div>Awesome!<br>
<br>
-static cl::opt<bool><br>
<div>-EnableGlobalMerge("global-merge", cl::Hidden,<br>
</div>- cl::desc("Enable global merge pass"),<br>
- cl::init(true));<br>
+static cl::opt<bool> EnableGlobalMerge("enable-global-merge", cl::NotHidden,<br>
+ cl::desc("Enable global merge pass"),<br>
+ cl::init(true));<br>
<br>
Please commit this option rename first to reduce the noise in the patch.<br></blockquote><div><br></div><div>This renaming is because the global option would be conflict with global pass enabling name otherwise, so the renaming itself can't justify why it needs to be changed,</div>
<div>but I'm fine to get it split from current patch if you want.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
+static cl::opt<bool> EnableGlobalMergeOnExternal(<br>
+ "global-merge-on-external", cl::Hidden,<br>
+ cl::desc("Enable global merge pass on external linkage"), cl::init(false));<br>
+<br>
<br>
This is a transitional option, right? I assume it will go away once it<br>
is tested and benchmarked. Please add a comment stating that if that<br>
is the case.<br></blockquote><div><br></div><div>I'm not sure it is a transitional option. It's harmless if we leave it there, right? The global merge benefit comes from target requirement. For example, for a target with few registers, we may not want to enlarge base address live range, although this is probably the same case for the original static global merge. So it would be easily for us to switch on/off for different targets.</div>
<div><br></div><div>However, I'm fine to add a comment like that if you want, and it doesn't hurt anything for me.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
-INITIALIZE_PASS(GlobalMerge, "global-merge",<br>
- "Global Merge", false, false)<br>
<br>
+static void *initializeGlobalMergePassOnce(PassRegistry &Registry) {<br>
+ PassInfo *PI = new PassInfo(<br>
</div>+ "Merge global variables", "global-merge", &GlobalMerge::ID,<br>
+ PassInfo::NormalCtor_t(callDefaultCtor<GlobalMerge>), false, false,<br>
+ PassInfo::TargetMachineCtor_t(callTargetMachineCtor<GlobalMerge>));<br>
<div>+ Registry.registerPass(*PI, true);<br>
+ return PI;<br>
+}<br>
+<br>
</div><div>+void llvm::initializeGlobalMergePass(PassRegistry &Registry) {<br>
+ CALL_ONCE_INITIALIZATION(initializeGlobalMergePassOnce)<br>
+}<br>
<br>
<br>
</div>Why is this necessary?<br></blockquote><div><br></div><div>This is because we want to register global merge statically, then opt can work with this pass. Otherwise, we wouldn't be able to check the correctness of this middle-end pass with opt. This is also why I move some test cases from CodeGen to Transforms.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ initializeGlobalMergePass(Registry);<br>
<br>
same here.<br>
<div><br>
<br>
+attributes #0 = { nounwind ssp "less-precise-fpmad"="false"<br>
"no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"<br>
"no-infs-fp-math"="false" "no-nans-fp-math"="false"<br>
"stack-protector-buffer-size"="8" "unsafe-fp-math"="false"<br>
"use-soft-float"="false" }<br>
+attributes #1 = { "less-precise-fpmad"="false"<br>
"no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"<br>
"no-infs-fp-math"="false" "no-nans-fp-math"="false"<br>
"stack-protector-buffer-size"="8" "unsafe-fp-math"="false"<br>
"use-soft-float"="false" }<br>
+attributes #2 = { nounwind readnone ssp "less-precise-fpmad"="false"<br>
"no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"<br>
"no-infs-fp-math"="false" "no-nans-fp-math"="false"<br>
"stack-protector-buffer-size"="8" "unsafe-fp-math"="false"<br>
"use-soft-float"="false" }<br>
+attributes #3 = { nounwind }<br>
+<br>
+!llvm.ident = !{!0}<br>
+<br>
+!0 = metadata !{metadata !"LLVM version 3.4 "}<br>
+!1 = metadata !{metadata !2, metadata !2, i64 0}<br>
+!2 = metadata !{metadata !"int", metadata !3, i64 0}<br>
+!3 = metadata !{metadata !"omnipotent char", metadata !4, i64 0}<br>
+!4 = metadata !{metadata !"Simple C/C++ TBAA"}<br>
<br>
</div>You probably don't need all this in the test.<br></blockquote><div><br></div><div>This is not introduced by my patch, but I'm OK to remove them with this patch if you think it's necessary.</div><div><br>
</div><div>Thanks,</div><div>-Jiangning</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>