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

[Philip Reames]

In http://reviews.llvm.org/D7121#123627, @hfinkel wrote:

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


If you write
if (LoadInst *LI = dyn_cast<LoadInst>(I))
or even:
if (isa<LoadInst>(I)) { LI = cast<LoadInst>(I); ... }

You are pretty much guaranteed that LI is in fact a load.

With ParseMemoryInst(I).isLoad() there's no self check.  You could see a path being added that set Load=true, but forgot to actually save the instruction, or similar things.

To be clear, I am *not* stating the current impl is buggy, just that its more error prone going forward.

> 

> 

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


Neither of these comments describe the *semantics*.  What is *meaning* or *usage* of the matching id?  Does it effect must aliasing rules?  (Serious question here, I can't tell what it actually means.)

> 

> 

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


Thanks.  A rename is definitely appropriate.

> 

> 

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


I think you're misreading my suggestion.  Here's some psuedo code:
class LoadSite {

  Instruction *Inst;
  LoadSite(Instruction* I);
  
  operator bool() const;
  
  bool isSimple() {
    if(LoadInst *LI = dyn_cast<LoadInst>) return LI->isSimple();
    assert is target intrinsic
    return TTI->getTarget...().isVolatile;

}
}

if (LoadSite LS = I) {
 ...
}
class StoreSite { ... };

if (StoreSite SS = I)

Having the abstraction is fine, it should just be minimal.  In particular, the abstraction itself shouldn't be stateful.

Given there's little shared between loads (of any type) and stores (of any type), I think these should be separate.


http://reviews.llvm.org/D7121

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