<div dir="ltr">Hi Rafael,<div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-11 1:58 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">>> +static cl::opt<bool> EnableGlobalMergeOnExternal(<br>
>> + "global-merge-on-external", cl::Hidden,<br>
>> + cl::desc("Enable global merge pass on external linkage"),<br>
>> 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>
><br>
><br>
> I'm not sure it is a transitional option. It's harmless if we leave it<br>
> there, right?<br>
<br>
</div>Not quiet. We don't like having too many switches. If this is<br>
something that is likely to be permanent (why?) it might be better to<br>
make it an option that is passed to the pass constructor.<br>
<div class=""><br>
> The global merge benefit comes from target requirement. For<br>
> example, for a target with few registers, we may not want to enlarge base<br>
> address live range, although this is probably the same case for the original<br>
> static global merge. So it would be easily for us to switch on/off for<br>
> different targets.<br>
><br>
> However, I'm fine to add a comment like that if you want, and it doesn't<br>
> hurt anything for me.<br>
<br>
</div>These are things that are better handled by a target switch. In this<br>
case, since this is just an on/off thing, it can probably be made into<br>
a pass constructor option and some targets can pass in true, other<br>
pass in false.<br>
<br>
For now this is only used for AArch64 and ARM, where the expected<br>
value is always true, so maybe the best is to just leave the option<br>
out?<br>
<br>
If you need it for debugging/testing during development, that is fine,<br>
but we should have a plan on how to remove it.<br></blockquote><div><br></div><div>The plan is to fix the so-called ADRP CSE issue in aarch64 back-end first, and then we can go back to clean and this switch and improve this pass if necessary.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
>><br>
>><br>
>> -INITIALIZE_PASS(GlobalMerge, "global-merge",<br>
>> - "Global Merge", false, false)<br>
>><br>
>> +static void *initializeGlobalMergePassOnce(PassRegistry &Registry) {<br>
>> + PassInfo *PI = new PassInfo(<br>
>> + "Merge global variables", "global-merge", &GlobalMerge::ID,<br>
>> + PassInfo::NormalCtor_t(callDefaultCtor<GlobalMerge>), false, false,<br>
>> + PassInfo::TargetMachineCtor_t(callTargetMachineCtor<GlobalMerge>));<br>
>> + Registry.registerPass(*PI, true);<br>
>> + return PI;<br>
>> +}<br>
>> +<br>
>> +void llvm::initializeGlobalMergePass(PassRegistry &Registry) {<br>
>> + CALL_ONCE_INITIALIZATION(initializeGlobalMergePassOnce)<br>
>> +}<br>
>><br>
>><br>
>> Why is this necessary?<br>
><br>
><br>
> This is because we want to register global merge statically, then opt can<br>
> work with this pass. Otherwise, we wouldn't be able to check the correctness<br>
> of this middle-end pass with opt. This is also why I move some test cases<br>
> from CodeGen to Transforms.<br>
<br>
</div>I guess the question is more, what is different from the existing<br>
macros? Other passes seem to just use a macro and are usable from opt.<br>
Looking a what the macro expands too, the difference is that you pass<br>
the target machine ctor too. Please add a comment saying that is the<br>
difference from what INITIALIZE_PASS would do.<br>
<br>
If you are really motivated an ever better change would be to add a<br>
INITIALIZE_TM_PASS macro :-)<br></blockquote><div><br></div><div>Actually I'm not the one creating this new usage, for example, CodeGenPreparePass is not using INITIALIZE_PASS directly either. I just simply followed the current example code in trunk. We use a special TargetMachineCtor just because we want to initialize TargetMachine, and some TargetLowering stuff needs to be invoked in this pass, because it is right now being invoked in PreISel stage.</div>
<div> </div><div>But anyway, I think your proposal of creating INITIALIZE_TM_PASS is good.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
On the new patches:<br>
<br>
* 0001-Rename-global-merge-to-enable-global-merge.patch<br>
<br>
Why do you need it to not be hidden? LGTM with it being hidden.<br></blockquote><div><br></div><div>OK. Committed as r210639.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I think patch 0002 is OK too with a comment about why the init macro<br>
is not used.<br></blockquote><div><br></div><div>Committed as revision r210640.</div><div><br></div><div>Finally I also committed r210641 by creating a separate macro INITIALIZE_TM_PASS, and three passes' initialization was simplified accordingly.</div>
<div><br></div><div>Thanks,</div><div>-Jiangning</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>