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

[Philip Reames]

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!)
- MatchingId - what does this even do?  It's not documented at all.
- isVolatile is confused with isSimple in problematic ways.  These are related but distinct concepts.
- 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.


http://reviews.llvm.org/D7121

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