<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=gb2312">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Hello,</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
I am interested in this if this improves SPEC, which I didn't know to be honest.</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
If this doesn't trigger anymore, I am willing to look into that (not immediately, but at some point), but<span style="color: rgb(0, 0, 0); font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; background: var(--white);"> if the opinion is that
the current state and implementation is beyond repair and need to be removed then that is interesting and probably best be discussed here as there are very few reviewers on those changes, so this message may not have reached all who are interested.</span></div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="color: rgb(0, 0, 0); font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; background: var(--white);"><br>
</span></div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="color: rgb(0, 0, 0); font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; background: var(--white);">Cheers,</span></div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="color: rgb(0, 0, 0); font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; background: var(--white);">Sjoerd. </span></div>
<div id="appendonsend"></div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> llvm-dev <llvm-dev-bounces@lists.llvm.org> on behalf of F¨¡ng-ru¨¬ S¨°ng via llvm-dev <llvm-dev@lists.llvm.org><br>
<b>Sent:</b> 18 May 2021 22:06<br>
<b>To:</b> Chris Lattner <clattner@nondot.org><br>
<b>Cc:</b> llvm-dev <llvm-dev@lists.llvm.org><br>
<b>Subject:</b> Re: [llvm-dev] Removal of Global SRoA</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">Hi Chris,<br>
<br>
[I took my second dose of vaccine yesterday and felt under the weather, so I may be slow on replies.]<br>
<br>
On 2021-05-18, Chris Lattner wrote:<br>
>Hi Fangrui,<br>
><br>
>In this patch you removed an optimization outright: <a href="https://reviews.llvm.org/rG129f466e222e">
https://reviews.llvm.org/rG129f466e222e</a><br>
><br>
>I didn¡¯t see this get discussed, and the only justification seems to be that ¡°it doesn¡¯t does not trigger at all when bootstrapping clang¡±.<br>
<br>
Sorry but I cannot agree with this. I opened<br>
<a href="https://reviews.llvm.org/D101245">https://reviews.llvm.org/D101245</a> (Apr 24) for review and the removal was<br>
my second attempt after I realized that it is non-trivial to address<br>
potential miscompilation issues.<br>
<br>
>This is a spec optimization, and while that may not be important to Google, that is important to many people who use LLVM. Can you please revert this patch and discuss if further?<br>
<br>
<a href="https://bugs.llvm.org/show_bug.cgi?id=50027">https://bugs.llvm.org/show_bug.cgi?id=50027</a> was reported by Loris Reiff,<br>
who is not associated with Google. I was helping fixing the issue as an<br>
LLVM community member. The bug reported by Loris could cause an internal<br>
compiler crash but I realized we could have miscompilation issues.<br>
<br>
<br>
I added abort() in PerformHeapAllocSRoA() and tested an internal spec2006 - no file caused a clang crash.<br>
So this optimization isn't triggered in spec2006.<br>
<br>
Arthur Eubanks confirmed this optimization did not fire when bootstrapping clang.<br>
<br>
I just checked llvm-test-suite as well - not triggered.<br>
<br>
Given that this optimization was not triggered in any of<br>
llvm-project/spec2006/llvm-test-suite and the optimization could cause<br>
miscompilation issues, I'd prefer we don't revert the patch.<br>
Once a regression is found, we can keep investigating.<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
llvm-dev@lists.llvm.org<br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</div>
</span></font></div>
</body>
</html>