PATCH: Early if-converter store if-conversion

Thu Apr 25 09:45:35 PDT 2013

On Apr 25, 2013, at 7:45 AM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:

> 
> On Apr 25, 2013, at 9:23 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
>> ----- Original Message -----
>>> From: "Arnold" <aschwaighofer at apple.com>
>>> To: "Andrew Trick" <atrick at apple.com>
>>> Cc: llvm-commits at cs.uiuc.edu
>>> Sent: Thursday, April 25, 2013 8:16:42 AM
>>> Subject: Re: PATCH: Early if-converter store if-conversion
>>> 
>>> 
>>> 
>>> Sent from my iPhone
>>> 
>>> On Apr 25, 2013, at 3:42 AM, Andrew Trick <atrick at apple.com> wrote:
>>> 
>>>> 
>>>> On Apr 23, 2013, at 1:22 PM, Arnold Schwaighofer
>>>> <aschwaighofer at apple.com> wrote:
>>>> 
>>>>> This patch performs a similar optimization than my previous
>>>>> attempt did in SimplifyCFG.
>>>>> 
>>>>> This time we convert conditional stores in the early-if converter
>>>>> where we can use trace metrics to gauge the “closeness” of the
>>>>> two stores. If they are likely to be execute close together the
>>>>> cost of the second store should be cheap compared to
>>>>> mispredicting a branch.
>>>>> 
>>>>> This patch needs more tests and probably a target lowering call
>>>>> whether the (sub)target wants to if convert stores. But I wanted
>>>>> to get peoples opinions/input. Also it is not polished yet (so
>>>>> feel free to ignore formatting and similar concerns).
>>>>> 
>>>>> I have (correctness) tested the patch on test-suite+external and
>>>>> "-mllvm -x86-early-ifcvt=true -mllvm -stress-early-ifcvt=true”
>>>>> and observed no correctness regressions.
>>>>> 
>>>>> Performance will have to be analysed (give that the previous patch
>>>>> in simplify cfg did not cause too many regressions I don’t expect
>>>>> many to crop up here - but you never know). Jakob said that there
>>>>> are still some performance regressions with the early-if
>>>>> converter (without my patch). So, I wanted to wait with
>>>>> performance analysis until they are resolved.
>>>>> 
>>>>> I have however confirmed that we still get the 20% in hmmer ;)
>>>>> 
>>>>> 
>>>>> Best,
>>>>> Arnold
>>>>> 
>>>>> Attached are two patches: the first adds an api to targetinstrinfo
>>>>> to query and modify “register to memory stores”. During
>>>>> if-conversion we need to change the value register of such stores
>>>>> and need to compare two stores’ memory locations. The second
>>>>> implements the if-conversion in the early-if conversion pass.
>>>>> 
>>>>> <0001-TargetInstrInfo-Add-api-to-query-and-change-simple-r.patch><0002-Early-if-conversion-Implement-if-conversion-of-store.patch>
>>>>> 
>>>>> 
>>>>> 
>>>>> The thread following my simplify cfg commit
>>>>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130415/172206.html
>>>>> has a discussion why we rather wanted to do this in simplify cfg.
>>>> 
>>>> It's sad that you needed new hooks specifically for this transform.
>>>> 
>>> 
>>> I believe that is the cost of doing transforms in codegen were you
>>> have target specific instructions and need to know their semantics.
>>> At least that has been my experience in the past.
>>> 
>>> Maybe it is not necessary in this case. I believe it is.
>>> 
>>>> I thought you could use mayStore along with an option to
>>>> isSafeToMove to allow stores. Then MachineMemOperand to determine
>>>> aliasing. Then maybe check if the aliasing stores have a single
>>>> vreg that differs and cmove on that?
>>> 
>>> 
>>> You are assuming that this vreg has the semantics of the value to be
>>> stored but it could mean something else. What about a theoretical
>>> "inc mem reg" increase a value in memory by register
>>> instruction.(without returning the value)
> 
> Would such an instruction have two machine memory operands (one for reading, one for writing)?
> 
> Anyway, I think we are in dangerous waters making assumptions about the semantics of an instructions by just looking at its operands, its may store property and having one vreg operand that is different from a preceding similar operation.
> 
> Also the memory operand does not qualify the set of machine instr operands that designate a memory location (on x86 it is 3 or so) I think. So we wouldn't know whether a differing vreg operand designates (part of the) memory address or the value being stored.

You're right. We can't assume a store is repeatable. I thought we could weed out weird cases by checking MachineMemOperand properties, but on second thought probably not.

We should have an MI abstraction that provides simple load/store semantics. It would be nice if that abstraction were minimal and reused across phases that care: if-convert, scheduler, load/store optimizer. It strikes me as odd that we need MachineMemOperands, getLdStBaseRegImmOfs, and haveRegToMemStoresSameLocation.

This shouldn't hold up the patch! I just want to eventually evolve toward a simpler API.

-Andy

>>> We have to make assumptions that a sane isa does not have such
>>> instructions then your approach probably works.
>> 
> 
> I shouldn’t be classifying ISAs as sane and not sane - I guess I am/was just a little grumpy this morning. Have not had my shot of coffee yet. ;)
> 
> - Yours truly,
> Austrian mountain troglodyte
> 
> 
> 
>> Hrmm... don't do that, there are such instructions. As another example, PPC has byte-swapping stores.
>> 
>> -Hal
>> 
>>> 
>>>> 
>>>> I could easily be missing something though...
>>>> 
>>>> -Andy
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130425/bd527656/attachment.html>