[PATCH] D19821: [EarlyCSE] Port to use MemorySSA (disabled by default). NFC.

George Burgess IV via llvm-commits llvm-commits at lists.llvm.org
Tue May 17 11:40:03 PDT 2016


> There are a couple of FIXME comments in this change that identify cases
where MemorySSA is maybe being too conservative (e.g. when dealing with
fence release instructions and load atomic instructions).

Yup! D16875 that should make MemSSA more willing to handle atomics/volatile
ops. Philip gave it a verbal LGTM*, but IIRC he thought it may be better to
wait until we get MSSA integrated into a few passes (to get any big bugs
worked out) before we start making it super aggressive with memory
ordering. I have no opinion on when it should land; if we want it in now,
I'm happy to commit it today.

> Similarly, what do you think of Phillip's suggestion to look at using
ValueHandles in MemorySSA to make removal invalidating more automated?

I was planning for that to come with the overhauled cache (most recent
attempt is abandoned at D19503). I should have more time to take another
stab at it this weekend. If not, I'll at least end up tweaking the current
implementation to use VHes.


* - (To be as clear as I can be, it was an LGTM with comments. Addressing
the comments required refactoring the code a bit. I'm assuming the
refactoring doesn't matter, and the LGTM still applies.)

On Tue, May 17, 2016 at 9:12 AM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> On Tue, May 17, 2016 at 8:23 AM, Geoff Berry <gberry at codeaurora.org>
> wrote:
>
>> gberry added a comment.
>>
>> @reames I've attempted to resolved most of your individual concerns (or
>> at least made them explicit in the change).  The bigger question of whether
>> this is worth the compile time remains to be determined.  Do you think more
>> tests need to be added in addition to the already existing EarlyCSE tests?
>> Adding additional run lines to those tests to enable
>> -early-cse-use-memoryssa seems like overkill to me, but I don't feel to
>> strongly about it.  Or are you more concerned about adding new tests for
>> cases that are only caught by MemorySSA (both positive and negative)?
>>
>> @dberlin, @george.burgess.iv There are a couple of FIXME comments in this
>> change that identify cases where MemorySSA is maybe being too conservative
>> (e.g. when dealing with fence release instructions and load atomic
>> instructions).
>
>
> This is known :)
>
>
>> Do you think it is reasonable to refine these cases in MemorySSA
>
>
> Yes, it's one of george's goals.
>
>
>>
>
> or is the conservatism restricted to EarlyCSE's usage, in which case we
>> should deal with it in EarlyCSE?
>
>
> No, we should make memoryssa work well.
>
>
>>   Similarly, what do you think of Phillip's suggestion to look at using
>> ValueHandles in MemorySSA to make removal invalidating more automated?
>>
>>
> Already suggested this to george a few weeks ago.
>
>
>>
>> http://reviews.llvm.org/D19821
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160517/815e66fb/attachment.html>


More information about the llvm-commits mailing list