<div dir="ltr"><div>Sorry, I missed the earlier suggestion about using an existing hook to guard this. <br></div><div><br></div><div>I think that is:</div><div> /// Return true if the specified immediate is legal add immediate, that is the<br> /// target has add instructions which can add a register with the immediate<br> /// without having to materialize the immediate into a register.<br> virtual bool isLegalAddImmediate(int64_t) const {<br> return true;<br> }<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 1, 2020 at 1:43 PM Sanjay Patel <<a href="mailto:spatel@rotateright.com">spatel@rotateright.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">That sounds reasonable to me. So to be clear - the case(s) you're looking at are related to gep expansion / address arithmetic, not general mul/add instructions in IR? I missed that from the original example. So we don't need an inverting transform, we just need to limit the backend.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jun 30, 2020 at 11:27 PM Ben Shi <<a href="mailto:powerman1st@163.com" target="_blank">powerman1st@163.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="line-height:1.7;color:rgb(0,0,0);font-size:14px;font-family:Arial"><div style="margin:0px">Thanks. I have checked the hook <span style="font-family:arial;white-space:pre-wrap">DAGCombiner::isMulAddWithConstProfitable</span></div><div style="margin:0px">And I think the above condition is too aggressive.</div><div style="margin:0px"><br></div><div style="margin:0px"><div style="margin:0px"> // If the add only has one use, this would be OK to do.</div><div style="margin:0px"> if (AddNode.getNode()->hasOneUse())</div><div style="margin:0px"> return true;</div><div><br></div><div>Shall we make it to </div></div><div style="margin:0px"> if (AddNode.getNode()->hasOneUse() && TargetLowering.isCheaperCommuteAddMul(......))</div><div style="margin:0px"> return true;</div><div style="margin:0px"><br></div><div style="margin:0px">The virtual hook nethod isCheaperCommuteAddMul will return true by default, but specific targets like arm/riscv can make their own decision.</div><div style="margin:0px"><br></div><div style="margin:0px">Just like virtual bool TargetLowering::decomposeMulByConstant</div><div style="margin:0px"><br></div><div style="margin:0px">What's your opinion?</div><div style="margin:0px"><br></div><div style="margin:0px">Ben</div><div style="margin:0px"><br></div><div></div><div id="gmail-m_-4482966413208381495gmail-m_5809721189910848008divNeteaseMailCard"></div><p style="margin:0px"><br></p><pre><br>At 2020-07-01 02:29:36, "Sam Elliott" <<a href="mailto:selliott@lowrisc.org" target="_blank">selliott@lowrisc.org</a>> wrote:
>The relevant DAGCombine is controlled by a hook: `DAGCombiner::isMulAddWithConstProfitable`.
>
>It may be that, when optimising for size, this hook should take into account whether `c1*c2` fits into an add immediate, something that the DAGCombiner has access to via TargetLoweringInfo (but does not often use).
>
>I imagine any change here could have far reaching consequences in terms of introducing changes across lots of targets - I'm not sure what the best approach is to handle that.
>
>Sam
>
>> On 30 Jun 2020, at 2:08 pm, Sanjay Patel via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:
>>
>> Yes - this has been in InstCombine for a long time:
>> <a href="https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp#L268" target="_blank">https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp#L268</a>
>>
>> We could say that the canonicalization should be reversed, but that probably uncovers more missing optimizations.
>>
>> The code size concern is legitimate. For example on x86, gcc asm is 2 bytes smaller on this example:
>> <a href="https://godbolt.org/z/GK9FEL" target="_blank">https://godbolt.org/z/GK9FEL</a>
>>
>> To improve this, we could add a generic transform to DAGCombiner to invert the transform that was done in IR. That transform would only be enabled with a TargetLowering hook that allows targets to decide if the constants or other factors (optimizing for size) make it worthwhile to reorder the ops.
>>
>> On Mon, Jun 29, 2020 at 11:54 PM Craig Topper via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:
>> This is likely more of a canonicalization than an optimization. This is done so that if you got the add followed by mul input or the mul followed by add input they would be canonicalized to the same sequence. Maybe not the optimal sequence but at least the same. I didn't check, but I suspect this is happening in InstCombine in the middle end.
>>
>> ~Craig
>>
>>
>> On Mon, Jun 29, 2020 at 8:37 PM Ben Shi via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:
>> Hi, James,
>>
>> Thanks for your reply.
>>
>> I do not think it is always true, that "mul then add" is faster than "add then mul".
>>
>> For example,
>>
>> A small immediate can be directly encoded in the instruction, but it becomes a larger one after a multiplication, which has to be loaded from the constant pool (extra memory access).
>>
>> So I wonder, is it possile to prevent it, via changes to the config of the base class TargetLowering，than writing special custom C++ code.
>>
>> Ben
>>
>>
>>
>>
>>
>>
>> At 2020-06-30 07:46:41, "James Courtier-Dutton" <<a href="mailto:james.dutton@gmail.com" target="_blank">james.dutton@gmail.com</a>
>> > wrote:
>> >Hi Ben,
>> >
>> >Why do you want to stop it?
>> >"mul then add" is faster than "add then mul".
>> >The result is the same in both cases.
>> >
>> >On Mon, 29 Jun 2020 at 22:11, Ben Shi via llvm-dev
>> ><
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
>> > wrote:
>> >>
>> >> Hello,
>> >> I have an instruction pattern like
>> >> %2 = add i32 %0, 25
>> >> %3 = mul i32 %2, 525
>> >>
>> >> and llvm will optimize it to
>> >> %2 = mul i32 %0, 525
>> >> %3 = add i32 %2, 12525
>> >>
>> >> how to prevent it?
>> >>
>> >> _______________________________________________
>> >> LLVM Developers mailing list
>> >>
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
>>
>> >>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
>> _______________________________________________
>> LLVM Developers mailing list
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
>> _______________________________________________
>> LLVM Developers mailing list
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
>> _______________________________________________
>> LLVM Developers mailing list
>> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
>
>--
>Sam Elliott
>Software Team Lead
>Senior Software Developer - LLVM and OpenTitan
>lowRISC CIC
</pre></div></blockquote></div>
</blockquote></div>