[PATCH][PowerPC] More refactoring prior to real PPC emitPrologue()/emitEpilogue() changes
Bill Schmidt
wschmidt at linux.vnet.ibm.com
Sun Aug 18 15:02:59 PDT 2013
Mark, I'll echo Hal's comment about the variable names. This is one of
those naming convention things that we all agree to live by.
In addition, please change:
STW_Inst to StoreInst
STWU_Inst to StoreUpdInst
STWUX_Inst to StoreUpdIdxInst
LWZ_Inst to LoadInst
or similar. The point is that STD is never a STW instruction, STDU is
never a STWU instruction, etc. Please change the names to avoid bias
for one subtarget or another.
Other than that, the patch looks fine. Please generate a new version
with the changed variable names and I will regression-test and commit
for you (provided nobody else has further comments).
BTW, thanks for doing this! Factoring this code is long overdue.
Thanks,
Bill
On Fri, 2013-08-16 at 17:03 -0500, Hal Finkel wrote:
> Mark,
>
> I'll look at this in more detail, but just a quick note: Please remove the underscores from the variable names.
>
> Thanks again,
> Hal
>
> ----- Original Message -----
> >
> >
> >
> >
> > This is a continuation of the refactorings performed in svn rev
> > 188573 (see that rev's comments for more detail).
> >
> >
> >
> > This is my stage 2 refactoring: I combined the emitPrologue() &
> > emitEpilogue() PPC32 & PPC64 code into a single flow, simplifying a
> > lot of the code since in essence the PPC32 & PPC64 code generation
> > logic is the same, only the instruction forms are different (in most
> > cases). This simplification is necessary because my functional
> > changes (yet to come) add significant complexity, and without the
> > simplification of my stage 2 refactoring, the overall complexity of
> > both emitPrologue() & emitEpilogue() would have become almost
> > intractable for most mortal programmers (like me).
> >
> >
> >
> > This submission was intended to be a pure refactoring (no functional
> > changes whatsoever). However, in the process of combining the PPC32
> > & PPC64 flows, I spotted a difference that I believe is a bug (see
> > svn rev 186478 line 863, or svn rev 188573 line 888): This line
> > appears to be restoring the BP with the original FP content, not the
> > original BP content. When I merged the 32-bit and 64-bit code, I
> > used the corresponding code from the 64-bit flow, which I believe
> > uses the correct offset (BPOffset) for this operation.
> >
> >
> >
> > http://llvm-reviews.chandlerc.com/D1430
> >
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
More information about the llvm-commits
mailing list