[PATCH] Commoning of target specific load/store intrinsics in Early CSE

[hfinkel at anl.gov]

In http://reviews.llvm.org/D7121#123664, @ssijaric wrote:

> Hi Philip,
>
> Thanks for the review.  Just to briefly add onto Hal's comment.
>
> In http://reviews.llvm.org/D7121#123602, @reames wrote:
>
> > - isVolatile is confused with isSimple in problematic ways.  These are related but distinct concepts.
>
>
> Yes, this should be renamed to avoid confusion.  There is no behavioral change, as Hal noted.


Okay, please do so.

> 

> 

> > - MatchingId - what does this even do? It's not documented at all.

> 

> 

> The reason for MatchingId is to identify matching loads and stores for target specific intrinsics.  For example, the pointer type on different target specific intrinsic calls may be i8*, and MatchingId is there to make sure that only matching intrinsics get commoned (e.g. test case test_nocse3).  This comment could be clearer.


I think the way to state this is that the action of a target-specific memory intrinsic is not uniquely identified by the pointer type. We need to make sure that magic permuting load of type 1 is only matched with magic permuting store of type 1, magic permuting load of type 1 is only matched with magic permuting store of type 2, etc.

> 

> 

> > - The usage of fields in the object appear unnecessary.  Simply dispatching by the contained instruction type would be far more clear and less error prone.

> 

> > 

> 

> >   A potentially clearer design would have been to introduce a helper class for each type of operation: load, and store.  Like CallSite, each helper class could simply proxy to the underlying instruction/information based on the type of the instruction so contained.

> 

> 

> I thought about introducing new classes to deal with both regular loads and target specific intrinsic loads (same for stores and target specific intrinsic stores).  This was only needed for EarlyCSE, so I kept it localized.

> 

> Looking back, it may have been cleaner to introduce these new classes, as they can be reused elsewhere (GVN?).  If everyone agrees,  I can come up with a new patch to do away with ParseMemInst.


I think this is worth trying.

> Thanks,

> Sanjin



http://reviews.llvm.org/D7121

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/