[llvm-commits] [PATCH, PowerPC] Fix MC instruction encodings

[Hal Finkel]

----- Original Message -----
> From: "Ulrich Weigand" <Ulrich.Weigand at de.ibm.com>
> To: llvm-commits at cs.uiuc.edu
> Sent: Monday, November 12, 2012 1:08:23 PM
> Subject: [llvm-commits] [PATCH, PowerPC] Fix MC instruction encodings
> 
> 
> Hello,
> 
> attempting to run the test-suite using the integrated assembler
> instead of
> the external assembler shows a couple of wrong instruction encodings.
>  The
> attached four patches fix all of those; if they're all applied (and
> the TOC
> order patch), there are no significant differences (at least in the
> .text
> section) between objects built with the external and the integrated
> assembler any longer, except for one remaining problem: the
> integrated
> assembler currently does not generate thread-local storage
> relocations.
> 
> The four patches fix the following problems:
> 
> 1. isel encoding
> The encoding for the isel instruction seems to have been completely
> wrong
> ever since the pattern was added.  The patch adds a new instruction
> format
> AForm_4 to handle this instruction.
> 
> 2. bd(n)z encoding
> This instruction likewise seems to have been wrong all along; it uses
> a
> format IForm_ext which does:
> 
> class IForm_ext<bits<6> opcode, bits<5> bo, bit aa, bit lk, dag OOL,
> dag
> IOL,
>             string asmstr, InstrItinClass itin, list<dag> pattern>
>          : IForm<opcode, aa, lk, OOL, IOL, asmstr, itin, pattern> {
>   let LI{0-4} = bo;
> }
> 
> This doesn't work since {0-4} affects the *low* bits of LI, and the
> value
> needs to be placed at the high bits.    In any case, it seems weird
> to use
> a variant of IForm for bd(n)z as those are prototypical BForm
> patterns; for
> example, the above pattern also does not attempt to set BI at all.
>   My
> patch changes this to use a variant of BForm for this instruction.
> 
> 3. Operand name mismatches
> In the past, is seems that instruction operands were matched onto
> field
> names in the instruction format strictly by order.  A while ago, a
> patch
> came in that changed this to try to look for a matching name first,
> and
> only use order to match those fields whose name doesn't match any
> operand
> name.  It appears the PowerPC back-end was never updated to
> accommodate
> that change.
> 
> Now, mostly this doesn't matter because none of the field names match
> any
> operand name anyway.  In some cases where there is a match, they so
> happen
> to occur in the correct order, so it doesn't hurt either.  However,
> there
> are a couple of places where now operands are matched in the wrong
> sequence.  The patch fixes this for the following formats:
>  - AForm_3  (fmul, fmuls)
>  - XFXForm_5 (mtcrf)
>  - XFLForm (mtfsf)
> 
> [I guess to avoid confusion, it might be good in the long term to
> change
> all operand names to match field names, so we never rely on order any
> more ...]
> 
> 4. Wrong opcodes
> A couple of encoding just plain use the wrong (major or minor)
> opcode, and
> thus generate the wrong instruction completely.   The patch fixes
> this for:
>  - lwaux
>  - lhzux
>  - stbu
> 
> 
> OK to commit?

LGTM. We should add test cases (but I think that we can't do that until we have the asm parser working, and that's blocked by TableGen problems), so those can wait.

Thanks again,
Hal

> 
> Bye,
> Ulrich
> (See attached file: diff-llvm-mc-encode-isel)
> (See attached file: diff-llvm-mc-encode-bdz)
> (See attached file: diff-llvm-mc-encode-opnames)
> (See attached file: diff-llvm-mc-encode-opcodes)
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory