[PATCH] D21246: [InstCombine] Add rule to fold away trunc of partial load

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: include/llvm/Analysis/Loads.h:82
@@ +81,3 @@
+/// This function returns value available at memory address *Ptr of type AccessTy.
+/// All sanity checks to confirm if this function should be called and whether
+/// the value returned can be used should be done at the caller.
----------------
anna wrote:
> sanjoy wrote:
> > I think "sanity checks" is too vague to be useful.  I think it is sufficient to state that we've assumed that we're folding a non-volatile unordered (but potentially atomic) load -- it is understood that people calling this function know what they want. :)
> I think we will miss some subtle points in that case, and will only be considering one usage of this function. 
> Basically, we do not make any assumptions on the memory operation, i.e. it need not be unordered non-volatile. This constraint is only required for the call from the `FindAvailableLoadedValue(LoadInst *Load,...)`. We do not need this constraint for the call from `InstCombiner::visitTrunc`, since we do not fold away the load.
> Also, the transformation using the value returned, can be a fold of the load as done in most cases, or fold of the trunc and nothing done to the load.
> 
> The idea is that it is up to the caller to decide what sort of memory operations they are interested in (may or may not be an unordered load) and what transformation needs to be done based on the value (may or may not be folding of load).
> 
> I thought this would be a very detailed explanation, and decided to state it as 'sanity checks' :)
> We do not need this constraint for the call from
> InstCombiner::visitTrunc, since we do not fold away the load.

I'm not sure if we are okay folding volatile loads, but I realized
(just now) that we're not okay folding atomic loads in
`visitTrunc`.  Our program could have been:

```
%ptr is initially 0

ThreadA:
  store atomic i32 1, i32* %ptr

ThreadB:
  %prevva = load atomic i16, (bitcast %ptr)
  %val = load atomic i32, i32* %ptr
  print(%val)
  print(trunc(%val))
```

Right now the program above can print `0, 0` or `1, 1`, but after your
change, `ThreadB` will be:


```
ThreadB:
  %prevva = load atomic i16, (bitcast %ptr)
  %val = load atomic i32, i32* %ptr
  print(%val)
  print(%prevva)
```

and will alllow the execution `1, 0` and `0, 1`.  I think we can fold
atomic loads only if the load has `trunc` as the only use.


http://reviews.llvm.org/D21246