<div dir="ltr">Your patch must pass all of the regression tests (everything under
llvm/test), or you'll get many fail mails from bots soon after
the commit. The commit will then be reverted if it can't be corrected immediately.<br><br>Running the test-suite is a good idea, but it's not required. If there's consensus that the patch is doing the theoretical right thing, then the patch should be ok to commit once reviewed and approved.<br><br>Particularly with low-level changes like this, it's possible that doing the right thing may still cause some unexpected change that leads to a regression on some hardware somewhere in the world. There are public and private bots running the test-suite and many other benchmarks, so you'll hear about it sooner or later if your patch makes something worse. :)<br><br>A couple of links to get started:<br><a href="https://llvm.org/docs/TestingGuide.html">https://llvm.org/docs/TestingGuide.html</a><br><a href="http://llvm.org/docs/lnt/quickstart.html">http://llvm.org/docs/lnt/quickstart.html</a><br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 19, 2017 at 12:35 PM, Haidl, Michael <span dir="ltr"><<a href="mailto:michael.haidl@uni-muenster.de" target="_blank">michael.haidl@uni-muenster.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<div dir="auto" style="direction:ltr;margin:0;padding:0;font-family:sans-serif;font-size:11pt;color:black;background-color:white">
Hi Sanjay,<br>
<br>
</div>
<div dir="auto" style="direction:ltr;margin:0;padding:0;font-family:sans-serif;font-size:11pt;color:black;background-color:white">
thanks for enlighten me on terms of tests. I assume I have to run the test-suite benchmarks to check for regressions? Is there a guide to get the metrics from the benchmarks?<br>
<br>
</div>
<div dir="auto" style="direction:ltr;margin:0;padding:0;font-family:sans-serif;font-size:11pt;color:black;background-color:white">
Cheers, <br>
</div>
<div dir="auto" style="direction:ltr;margin:0;padding:0;font-family:sans-serif;font-size:11pt;color:black;background-color:white">
Michael<br>
<br>
</div>
<div dir="auto" style="direction:ltr;margin:0;padding:0;font-family:sans-serif;font-size:11pt;color:black;background-color:white">
BTW the beginner tag for bugs was really a good idea to get started with contributing to llvm.</div><div><div class="h5">
<br>
<br>
<br>
<div class="gmail_quote">On Tue, Sep 19, 2017 at 3:58 PM +0200, "Sanjay Patel" <span dir="ltr">
<<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="3D"ltr"">
<div dir="ltr">
<div>
<div>For the tests that are changing, you should see if those changes are improvements, regressions, or neutral. This is unfortunately not always obvious for x86 asm, so feel free to just post those diffs in an updated version of the patch at D37896.<br>
<br>
</div>
If the test files have auto-generated assertions (look for this string on the first line of the test file: "NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.p<wbr>y"...<br>
and both of these do as of: <a href="https://reviews.llvm.org/rL313631" rel="noreferrer" target="_blank">
https://reviews.llvm.org/rL313<wbr>631</a> ), then it's easy to observe the diffs by re-running that script after your code patch is applied:<br>
$ /path/to/update_llc_test_check<wbr>s.py --llc=/path/to/local/and/new/l<wbr>lc lea-3.ll<br>
</div>
$ svn diff lea-3.ll<br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Tue, Sep 19, 2017 at 5:23 AM, Haidl, Michael via llvm-dev
<span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I am currently improving the D37896 to include the suggestions from<br>
Chad. However, running the lit checks for the x86 backend I observe some<br>
changes in the generated MC, e.g.:<br>
<br>
llvm/test/CodeGen/X86/lea-3.ll<wbr>:13:10: error: expected string not found<br>
in input<br>
; CHECK: leal ([[A0]],[[A0]],2), %eax<br>
^<br>
<stdin>:10:2: note: scanning from here<br>
orq %rdi, %rax<br>
^<br>
<stdin>:10:2: note: with variable "A0" equal to "%rdi"<br>
orq %rdi, %rax<br>
^<br>
<stdin>:10:2: note: with variable "A0" equal to "%rdi"<br>
orq %rdi, %rax<br>
^<br>
<stdin>:23:2: note: possible intended match here<br>
leal (,%rdi,4), %eax<br>
^<br>
or:<br>
<br>
llvm/test/CodeGen/X86/mul-cons<wbr>tant-i16.ll:40:13: error: expected string<br>
not found in input<br>
; X86-NEXT: movzwl {{[0-9]+}}(%esp), %eax<br>
^<br>
<stdin>:35:2: note: scanning from here<br>
movzwl 4(%esp), %ecx<br>
^<br>
llvm/test/CodeGen/X86/mul-cons<wbr>tant-i16.ll:272:13: error: expected string<br>
not found in input<br>
; X86-NEXT: movzwl {{[0-9]+}}(%esp), %eax<br>
^<br>
<stdin>:212:2: note: scanning from here<br>
movzwl 4(%esp), %ecx<br>
^<br>
<br>
What is the right way to fix this? Is it ok to modify the tests to match<br>
the new generated pattern?<br>
<br>
Cheers,<br>
Michael<br>
<span><br>
<br>
Am 16.09.2017 um 15:46 schrieb Simon Pilgrim:<br>
> This conversation has (partially) moved on to D37896 now, but if<br>
> possible I was hoping that we could perform this in DAGCombiner and<br>
> remove the various target specific combines that we still have.<br>
><br>
> At least ARM/AARCH64 and X86 have cases that can hopefully be<br>
> generalised and removed, but there will probably be a few legality/perf<br>
> issues that will occur.<br>
><br>
> Simon.<br>
><br>
>> On 14 Sep 2017, at 06:23, Craig Topper <<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a><br>
</span><span>>> <mailto:<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a><wbr>>> wrote:<br>
>><br>
>> Probably in visitMUL in DAGCombiner.cpp to be target independent. Or<br>
>> in LowerMUL in X86ISelLowering.cpp to be X86 specific.<br>
>><br>
>> Adding Simon. Simon, which were you thinking?<br>
>><br>
>> ~Craig<br>
>><br>
>> On Wed, Sep 13, 2017 at 10:06 PM, Haidl, Michael<br>
</span>>> <<a href="mailto:michael.haidl@uni-muenster.de" target="_blank">michael.haidl@uni-muenster.de</a> <mailto:<a href="mailto:michael.haidl@uni-muenster.de" target="_blank">michael.haidl@uni-muen<wbr>ster.de</a>>><br>
<span>>> wrote:<br>
>><br>
>> Hi Craig,<br>
>><br>
>> thanks for digging into this. So InstCombine is the wrong place for<br>
>> fixing PR34474. Can you give me a hint where such an optimization<br>
>> should<br>
>> go into CodeGen? I am not really familiar with stuff that happens<br>
>> after<br>
>> the MidLevel.<br>
>><br>
>> Cheers,<br>
>> Michael<br>
>><br>
>> Am 13.09.2017 um 19:21 schrieb Craig Topper:<br>
>> > And that is less instructions. So from InstCombine's perspective the<br>
>> > multiply is the correct answer. I think this transformation is better<br>
>> > left to codegen where we know whether multiply or shift is truly better.<br>
>> ><br>
>> > ~Craig<br>
>> ><br>
>> > On Wed, Sep 13, 2017 at 10:18 AM, Craig Topper <<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a> <mailto:<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a><wbr>><br>
</span><span>>> > <mailto:<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a> <mailto:<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a><wbr>>>> wrote:<br>
>> ><br>
>> > There is in fact a transform out there somewhere that reverses yours.<br>
>> ><br>
>> > define i64 @foo(i64 %a) {<br>
>> > %b = shl i64 %a, 5<br>
>> > %c = add i64 %b, %a<br>
>> > ret i64 %c<br>
>> > }<br>
>> ><br>
>> > becomes<br>
>> ><br>
>> > define i64 @foo(i64 %a) {<br>
>> ><br>
>> > %c = mul i64 %a, 33<br>
>> ><br>
>> > ret i64 %c<br>
>> ><br>
>> > }<br>
>> ><br>
>> ><br>
>> > ~Craig<br>
>> ><br>
>> > On Wed, Sep 13, 2017 at 10:11 AM, Craig Topper<br>
>> > <<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a> <mailto:<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a><wbr>><br>
</span>>> <mailto:<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.<wbr>com</a> <mailto:<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a><wbr>>>><br>
<span>>> wrote:<br>
>> ><br>
>> > Your code seems fine. InstCombine can infinite loop if some<br>
>> > other transform is reversing your transform. Can you send the<br>
>> > whole patch and a test case?<br>
>> ><br>
>> > ~Craig<br>
>> ><br>
>> > On Wed, Sep 13, 2017 at 10:01 AM, Haidl, Michael via llvm-dev<br>
>> > <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
</span>>> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.o<wbr>rg</a>> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.or<wbr>g</a><br>
<div class="m_4750937173548675184HOEnZb">
<div class="m_4750937173548675184h5">>> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.o<wbr>rg</a>>>> wrote:<br>
>> ><br>
>> > Hi,<br>
>> ><br>
>> > I am working on PR34474 and try to add a new<br>
>> optimization to<br>
>> > InstCombine. Like in other parts of the visitMul<br>
>> function I<br>
>> > add a Shl<br>
>> > through the IR builder and create a new BinaryOp which I<br>
>> > return from<br>
>> > visitMul. If I understand correctly the new BinaryOp<br>
>> > returned from<br>
>> > visitMul should replace the original Instruction in the<br>
>> > Worklist.<br>
>> > However, I end up in an infinite loop and the<br>
>> Instruction I<br>
>> > try to<br>
>> > replace gets scheduled again and again. What is<br>
>> wrong in my<br>
>> > code?<br>
>> ><br>
>> > // Replace X * (2^C+/-1) with (X << C) -/+ X<br>
>> > APInt Plus1 = *IVal + 1;<br>
>> > APInt Minus1 = *IVal - 1;<br>
>> > int isPow2 = Plus1.isPowerOf2() ? 1 :<br>
>> Minus1.isPowerOf2() ?<br>
>> > -1 : 0;<br>
>> ><br>
>> > if (isPow2) {<br>
>> > APInt &Pow2 = isPow2 > 0 ? Plus1 : Minus1;<br>
>> > Value *Shl = Builder.CreateShl(Op0,<br>
>> Pow2.logBase2());<br>
>> > return BinaryOperator::Create(isPow2 > 0 ?<br>
>> > BinaryOperator::Sub :<br>
>> > BinaryOperator::Add, Shl, Op0);<br>
>> > }<br>
>> ><br>
>> > Thanks,<br>
>> > Michael<br>
>><br>
><br>
</div>
</div>
<div class="m_4750937173548675184HOEnZb">
<div class="m_4750937173548675184h5">______________________________<wbr>_________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-dev</a><br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</blockquote>
</div>
</div></div></div>
</blockquote></div><br></div>