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

[Sanjin Sijaric]

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.

> - 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.

> - 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.

Thanks,
Sanjin


http://reviews.llvm.org/D7121

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