[llvm-commits] [PATCH] Efficient implementation of ITState tracking in Thumb disassembler

[Richard Barton]

Hi Bill

My patch yesterday did not contain all the edits that I promised. The new patch attached should be right.

Sorry about that.

Rich

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Richard Barton
> Sent: 17 April 2012 16:20
> To: Bill Wendling
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Efficient implementation of ITState
> tracking in Thumb disassembler
>
> Hi Bill
>
> Thanks for the review, comments inline and new patch attached
>
> > 1) We don't normally use 'std::stack' but a 'std::vector' and pop and push
> > onto the end.
> >
> > 2) Please put the class inside of an anonymous namespace.
> >
> > 3) You don't need to have an empty c'tor and d'tor.
> >
> > 4) Your 'instrLastInITBlock' method calls 'instrInITBlock', but the only
> place
> > the method is used it already calls this function. And it's not really
> > necessary to check 'instrInITBlock' because the .size() will return 0 if
> > ITStats is empty. :)
> >
>
> Done.
>
> > 5) You changed 'InstPrinter/ARMInstPrinter.cpp' to not print out '<und>'.
> > What's the purpose of this change?
> >
>
> The invalid-IT-CC15.txt test expects the inst printer to create e.g.
>
> ite al
> vldr d19, [pc, #388]
> vldr<und> d16, [pc, #384]
>
> My patch handles the CC = 15 case for VFP instructions in the same way as for
> normal Thumb2 instructions by making such instructions AL. I'm not sure if the
> <und> thing is a GNU as-ism, it works around an assertion failure (caused when
> the opposite condition code to AL is sought.)
>
> I don't think that the behaviour should be different between T2 VFP
> instructions and normal instructions here, so the code should be changed. We
> can kind of do what we like here as IT AL is unpredictable, so have a few
> options:
>
> 1. Revert T2 VFP instruction behaviour to the T2 normal instruction behaviour
> (syserr)
> 2. Revert T2 normal instruction behaviour to the T2 VFP instruction behaviour
> (this <und> thing)
> 3. Something else
>
> I think if we go with 2. then we should add a new condition code in the inst
> printer to mean the opposite of AL (NV?) and handle this properly. These
> instructions will always be unpredictable, but MC should be able to reason
> about them. That will form a separate patch as you suggest below.
>
> For the meantime, I will revert the patched behaviour to the previous
> behaviour with a difference between T2 instructions and VFP instructions.
>
> > 6) You changed 'CondBit0 = Mask >> 4 & 1' to 'CondBit0 = Firstcond & 1'.
> Why?
>
> Mask is the bottom 4 bits of the instruction, firstcond is the next 4.
> CondBit0 is the bottom bit of firstcond, so I changed the code to reflect what
> is really going on here. It seems that the  decoder cheekily adds the bottom
> bit of firstcond to the top of the mask, claiming this to be necessary for the
> inst printer to work.
> ...
> void ARMInstPrinter::printThumbITMask(const MCInst *MI, unsigned OpNum,
>                                       raw_ostream &O) {
>   // (3 - the number of trailing zeros) is the number of then / else.
>   unsigned Mask = MI->getOperand(OpNum).getImm();
>   unsigned CondBit0 = Mask >> 4 & 1;
> ...
>
> This seems wrong to me, I think that we can change the inst printer to get
> firstcond by doing arithmetic on OpNum (above) in a similar way to
> ARMInstPrinter::printT2SOOperand (for example.)
>
> Again, this needs another patch, so I will remove the change from this
> refactoring patch.
>
> > I would suggest refactoring first -- creating the class, etc. -- without
> > changing the code. Then you can convert from pushing onto the front of a
> > std::vector (which...gross!) to a stack-based approach.
>
> With the above comments, I believe the new attached patch should be a solely
> refactoring patch.
>
> I hope to submit a few more patches to clean up the remaining issues later.
> Would you be happy to review these also?
>
> Thanks
> Rich
>
>
> > -----Original Message-----
> > From: Bill Wendling [mailto:wendling at apple.com]
> > Sent: 13 April 2012 21:40
> > To: Richard Barton
> > Cc: llvm-commits at cs.uiuc.edu
> > Subject: Re: [llvm-commits] [PATCH] Efficient implementation of ITState
> > tracking in Thumb disassembler
> >
> > Hi Richard,
> >
> > If this is a pure refactoring, then it LGTM. There are few things:
> >
> > 1) We don't normally use 'std::stack' but a 'std::vector' and pop and push
> > onto the end.
> >
> > 2) Please put the class inside of an anonymous namespace.
> >
> > 3) You don't need to have an empty c'tor and d'tor.
> >
> > 4) Your 'instrLastInITBlock' method calls 'instrInITBlock', but the only
> place
> > the method is used it already calls this function. And it's not really
> > necessary to check 'instrInITBlock' because the .size() will return 0 if
> > ITStats is empty. :)
> >
> > 5) You changed 'InstPrinter/ARMInstPrinter.cpp' to not print out '<und>'.
> > What's the purpose of this change?
> >
> > 6) You changed 'CondBit0 = Mask >> 4 & 1' to 'CondBit0 = Firstcond & 1'.
> Why?
> >
> > I would suggest refactoring first -- creating the class, etc. -- without
> > changing the code. Then you can convert from pushing onto the front of a
> > std::vector (which...gross!) to a stack-based approach.
> >
> > -bw
> >
> > On Apr 13, 2012, at 6:32 AM, Richard Barton <richard.barton at arm.com> wrote:
> >
> > > Ping
> > > ________________________________
> > > From: llvm-commits-bounces at cs.uiuc.edu [llvm-commits-bounces at cs.uiuc.edu]
> On
> > Behalf Of Richard Barton [Richard.Barton at arm.com]
> > > Sent: 02 April 2012 23:47
> > > To: llvm-commits at cs.uiuc.edu
> > > Subject: Re: [llvm-commits] [PATCH] Efficient implementation of ITState
> > tracking        in Thumb disassembler
> > >
> > > Just bumping my patch, probably it got lost over the weekend.
> > >
> > >> -----Original Message-----
> > >> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > >> bounces at cs.uiuc.edu] On Behalf Of Richard Barton
> > >> Sent: 29 March 2012 18:53
> > >> To: llvm-commits at cs.uiuc.edu
> > >> Subject: [llvm-commits] [PATCH] Efficient implementation of ITState
> > tracking
> > >> in Thumb disassembler
> > >>
> > >> Hello reviewers
> > >>
> > >> The attached patch refactors the disassembly of Thumb IT instructions.
> The
> > IT
> > >> state is pushed to a stack and then popped and applied to subsequent
> > >> instructions in the IT block.
> > >>
> > >> The old implementation used an insertion at the front of a vector which
> is
> > >> linear in complexity to the size of the vector. My patch replaces it with
> > an
> > >> actual stack, and encapsulates it in a class.
> > >>
> > >> Please review.
> > >>
> > >> Thanks
> > >>
> > >> Richard Barton
> > >
> > > -- IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended
> > recipient, please notify the sender immediately and do not disclose the
> > contents to any other person, use it for any purpose, or store or copy the
> > information in any medium.  Thank you.
> > >
> > > -- IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended
> > recipient, please notify the sender immediately and do not disclose the
> > contents to any other person, use it for any purpose, or store or copy the
> > information in any medium.  Thank
> >
> you.<sdcomp_18512_IT_state_refactor.patch>____________________________________
> > ___________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-
> > commits_______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium.  Thank you.

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IT_state_disassembly_refactoring.patch
Type: application/octet-stream
Size: 6108 bytes
Desc: IT_state_disassembly_refactoring.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120418/9fcb4fda/attachment.obj>