[PATCH] [InstructionMerge - GVN] hoisting and sinking of equivalent memory instructions in diamonds
listmail at philipreames.com
Tue Jun 10 10:17:40 PDT 2014
Before anything else, the naming and documentation of this pass need
improved. I have no idea what "IM" stands for and the file level
documentation doesn't say. I finally got down to the pass registration
and found "Instruction Merger". This is far too vague to be useful.
For code comments, please put this on phabricator. I see a number of
small issues which need addressed.
This code seems like it duplicates lots of legality tests from other
places in the code base. I don't see obvious ommisions, but the code
duplication is questionable. Could you refactor out a single shared copy?
More generally, there's a lot of shared logic which other passes which
perform code hoisting and sinking (LICM for one). Why does this need to
be it's own pass rather than an extension to something existing?
On 06/09/2014 08:42 PM, Gerolf Hoflehner wrote:
> Alright, time has come to move on with this one. The instruction merge
> code is now a pass eagerly awaiting expert review eyes.
> On May 2, 2014, at 5:13 PM, Chandler Carruth <chandlerc at google.com
> <mailto:chandlerc at google.com>> wrote:
>> On Fri, May 2, 2014 at 4:44 PM, Gerolf Hoflehner
>> <ghoflehner at apple.com <mailto:ghoflehner at apple.com>> wrote:
>> I can see the arguments pro independent pass, but why do you
>> assert it will make the code much easier to review? The code is
>> coherent as it is and won't change whether it is part of GVN or
>> its own phase.
>> Because then I don't have to even think about GVN? Anyways, it should
>> be *much* easier to test.
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits