<html><body><p><font size="2">Thank you for the advice.</font><br><font size="2">InstCombiner::SliceUpIllegalIntegerPHI splits only when the output of the PHI node is only used by trunc or lshr</font><font size="2">+trunc (and the integer size is not legal).</font><br><font size="2">So it cannot not handle update to the field. If we extend SliceUpIllegalIntegerPHI to support more generic cases, I feel it may become similar to the split functionality in SROA.</font><br><br><font size="2">Extending the split functionality (in SROA or InstCombiner) will give higher coverage but it requires complicated (and potentially costly) analysis.</font><br><font size="2">My approach of splitting arguments in SROA preprocessing aim to cover a common case without such complicated analysis.</font><br><font size="2">Which approach is more preferable/acceptable for the community?</font><br><br><br><font size="2">[FYI] </font><br><font size="2">Here, I attach the IRs generated by SROA for the example in my first mail.</font><br><font size="2">The output of PHI (%r.sroa.2.0) is used by the and instruction. </font><br><br><font size="2">define signext i32 @_Z4func6record([2 x i64] %r.coerce) #0 {</font><br><font size="2">entry:</font><br><font size="2"> %r.coerce.fca.0.extract = extractvalue [2 x i64] %r.coerce, 0</font><br><font size="2"> %r.coerce.fca.1.extract = extractvalue [2 x i64] %r.coerce, 1</font><br><font size="2"> br label %for.cond</font><br><br><font size="2">for.cond: ; preds = %for.body, %entry</font><br><font size="2"> %r.sroa.2.0 = phi i64 [ %r.coerce.fca.1.extract, %entry ], [ %r.sroa.2.8.insert.insert, %for.body ]</font><br><font size="2"> %i.0 = phi i32 [ 0, %entry ], [ %inc1, %for.body ]</font><br><font size="2"> %conv = sext i32 %i.0 to i64</font><br><font size="2"> %cmp = icmp slt i64 %conv, %r.coerce.fca.0.extract</font><br><font size="2"> br i1 %cmp, label %for.body, label %for.cond.cleanup</font><br><br><font size="2">for.cond.cleanup: ; preds = %for.cond</font><br><font size="2"> %r.sroa.2.8.extract.trunc6 = trunc i64 %r.sroa.2.0 to i32</font><br><font size="2"> ret i32 %r.sroa.2.8.extract.trunc6</font><br><br><font size="2">for.body: ; preds = %for.cond</font><br><font size="2"> %r.sroa.2.8.extract.trunc = trunc i64 %r.sroa.2.0 to i32</font><br><font size="2"> %inc = add nsw i32 %r.sroa.2.8.extract.trunc, 1</font><br><font size="2"> %r.sroa.2.8.insert.ext = zext i32 %inc to i64</font><br><font size="2"> %r.sroa.2.8.insert.mask = and i64 %r.sroa.2.0, -4294967296</font><br><font size="2"> %r.sroa.2.8.insert.insert = or i64 %r.sroa.2.8.insert.mask, %r.sroa.2.8.insert.ext</font><br><font size="2"> %inc1 = add nsw i32 %i.0, 1</font><br><font size="2"> br label %for.cond</font><br><font size="2">}</font><br><font size="2"><br>-----<br>Hiroshi Inoue <inouehrs@jp.ibm.com><br>IBM Research - Tokyo<br></font><br><br><tt><font size="2">"Friedman, Eli" <efriedma@codeaurora.org> wrote on 2017/05/10 02:53:09:<br><br>> From: "Friedman, Eli" <efriedma@codeaurora.org></font></tt><br><tt><font size="2">> To: Hiroshi 7 Inoue/Japan/IBM@IBMJP, llvm-dev@lists.llvm.org</font></tt><br><tt><font size="2">> Date: 2017/05/10 02:54</font></tt><br><tt><font size="2">> Subject: Re: [llvm-dev] RFC: SROA for method argument</font></tt><br><tt><font size="2">> <br>> When there are accesses of mixed type to an alloca, SROA just treats<br>> the whole alloca as a big integer, and generates PHI nodes <br>> appropriately. In many cases, instcombine would then slice up the <br>> generated PHI nodes to use more appropriate types, but that doesn't <br>> work out here. (See InstCombiner::SliceUpIllegalIntegerPHI.) <br>> Probably the right solution is to make instcombine more aggressive <br>> here; it's hard to come up with a generally useful transform in SROA<br>> without reasoning about control flow.</font></tt><br><tt><font size="2">> -Eli</font></tt><br><tt><font size="2">> -- <br>> Employee of Qualcomm Innovation Center, Inc.<br>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a<br>> Linux Foundation Collaborative Project</font></tt><br><tt><font size="2">></font></tt><br><tt><font size="2">> On 5/9/2017 6:05 AM, Hiroshi 7 Inoue via llvm-dev wrote:</font></tt><br><tt><font size="2">> Hi,<br>> <br>> I am working to improve SROA to generate better code when a method <br>> has a struct in its arguments. I would appreciate it if I could have<br>> any suggestions or comments on how I can best proceed with this optimization.<br>> <br>> * Problem *<br>> I observed that LLVM often generates redundant instructions around <br>> glibc’s istreambuf_iterator. The problem comes from the scalar <br>> replacement (SROA) for methods with an aggregate as an argument. <br>> Here is a simplified example in C. <br>> <br>> struct record {<br>> long long a;<br>> int b;<br>> int c;<br>> };<br>> <br>> int func(struct record r) {<br>> for (int i = 0; i < r.c; i++)<br>> r.b++;<br>> return r.b;<br>> }<br>> <br>> When updating r.b (or r.c as well), SROA generates redundant <br>> instructions on some platforms (such as x86_64 and ppc64); here, r.b<br>> and r.c are packed into one 64-bit GPR when the struct is passed as <br>> a method argument. The problem is caused when the same memory <br>> location is accessed by load/store instructions of different types.<br>> For this example, CLANG generates following IRs to initialize the <br>> struct for ppc64 and x86_64. For both platforms, the 64-bit value is<br>> stored into memory allocated by alloca first. Later, the same memory<br>> location is accessed as 32-bit integer values (r.b and r.c).<br>> <br>> for ppc64<br>> %struct.record = type { i64, i32, i32 }<br>> <br>> define signext i32 @ppc64le_func([2 x i64] %r.coerce) #0 {<br>> entry:<br>> %r = alloca %struct.record, align 8<br>> %0 = bitcast %struct.record* %r to [2 x i64]*<br>> store [2 x i64] %r.coerce, [2 x i64]* %0, align 8<br>> ....<br>> <br>> for x86_64<br>> define i32 @x86_64_func(i64 %r.coerce0, i64 %r.coerce1) #0 {<br>> entry:<br>> %r = alloca %struct.record, align 8<br>> %0 = bitcast %struct.record* %r to { i64, i64 }*<br>> %1 = getelementptr inbounds { i64, i64 }, { i64, i64 }* %0, i32 0, i32 0<br>> store i64 %r.coerce0, i64* %1, align 8<br>> %2 = getelementptr inbounds { i64, i64 }, { i64, i64 }* %0, i32 0, i32 1<br>> store i64 %r.coerce1, i64* %2, align 8<br>> ....<br>> <br>> For such code sequence, the current SROA generates instructions to <br>> update only upper (or lower) half of the 64-bit value when storing <br>> r.b (or r.c). SROA can split an i64 value into two i32 values under <br>> some conditions (e.g. when the struct contains only int b and int c <br>> in this example), but it is not capable of splitting complex cases.</font></tt><br><BR>
</body></html>