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

[Bill Wendling]

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