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

[Anna Thomas via llvm-commits]

anna marked 2 inline comments as done.

================
Comment at: include/llvm/Analysis/Loads.h:80
@@ +79,3 @@
+/// is found, it is left unmodified.
+///
+/// This function returns value available at memory address *Ptr of type AccessTy.
----------------
sanjoy wrote:
> Describe `isAtomicMemOp`, and note that we're constant folding an unordered load (actually, maybe that's a better name for this function - `FoldLoadFromAddress`?).
> 
> Also, by convention, it should be `IsAtomicMemOp`.
Described `IsAtomicMemOp` as 

```
IsAtomicMemOp specifies the atomicity of the memory operation that accesses \p *Ptr. We verify atomicity constraints are satisfied when value forwarding from another memory operation that has value \p *Ptr available. 
```


But `FoldLoadFromAddress` is perhaps a misleading name, since the transformation done by the caller need not be folding of load (explained below). 

================
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.
----------------
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' :)


http://reviews.llvm.org/D21246