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

[hfinkel at anl.gov]

In http://reviews.llvm.org/D7121#123602, @reames wrote:

> Sorry to reopen a long dead review thread, but I had a reason to look at EarlyCSE today and just noticed the changes.
>
> I feel the introduction of the ParseMemoryInst class has badly obscured the logic of this transform.  I get that you wanted to abstract over the existence of both normal loads and stores and target specific loads and stores, but the extra layer of abstraction makes this code substantially harder to follow.
>
> Particular areas of concern include:
>
> - isValid checks as opposed to dyn_cast, cast style tests (i.e. less error checking!)


Can you please provide an example of what you mean?

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


There is a comment in the TTI header explaining what this does:

  // Same Id is set by the target for corresponding load/store intrinsics.
  unsigned short MatchingId;

and there is a comment in the implementation too:

  // For regular (non-intrinsic) loads/stores, this is set to -1. For
  // intrinsic loads/stores, the id is retrieved from the corresponding
  // field in the MemIntrinsicInfo structure.  That field contains
  // non-negative values only.
  int MatchingId;

> - isVolatile is confused with isSimple in problematic ways.  These are related but distinct concepts.


I assume you're talking about this:

  Vol = !LI->isSimple();

FWIW, I don't believe there was a behavioral change here. We can certainly re-name these variables.

> - 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'm not sure. There is common behavior between load intrinsics and loads, store intrinsics and stores, etc.


http://reviews.llvm.org/D7121

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