PATCH: Early if-converter store if-conversion

[Hal Finkel]

----- Original Message -----
> From: "Arnold Schwaighofer" <aschwaighofer at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu, "Andrew Trick" <atrick at apple.com>
> Sent: Thursday, April 25, 2013 9:45:18 AM
> Subject: Re: PATCH: Early if-converter store if-conversion
> 
> 
> 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)?

No, but one would need to be careful not to combine it with a regular write to the same address.

> 
> 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.

I agree. There can also be late-expanded pseudos that do crazy things ;)

 -Hal

> 
> 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.
> 
> >> 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
> 
>