[PATCH] Update InstCombine to transform aggregate stores into scalar stores.

Amaury SECHET deadalnix+llvmreview at gmail.com
Fri Mar 6 16:07:27 PST 2015


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:811
@@ +810,3 @@
+static bool unpackStoreToAggregate(InstCombiner &IC, StoreInst &SI) {
+  // FIXME: We could probably with some care handle both volatile and atomic
+  // stores here but it isn't clear that this is important.
----------------
reames wrote:
> Rather than returning bool, please return the new Instruction or nullptr.  This will allow us to add that new instruction to the worklist and will solve the deeply nested struct thing you were trying to address with recursion in the previous patch.  
> 
> Alternatively, you could simple add the newly created store to the worklist within this function.  I have no strong preference, but I suspect Chandler may care.  It might be good to let him comment before actually making either change.
None of this is not required, the builder sue a custom inserted that do it already. As for the bool return, I followed convention existing in the file.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:825
@@ +824,3 @@
+    if (ST->getNumElements() == 1) {
+      V = IC.Builder->CreateExtractValue(V, 0);
+      combineStoreToNewValue(IC, SI, V);
----------------
reames wrote:
> Add a comment here that highlights the fact that the wrapped value can be smaller than the FCA and that we are possibly loosing dereferencability information here.  We're okay with that for now, but it would be good to have it documented.
It does not appear to be possible in the case where we have struct with one element.

http://reviews.llvm.org/D7780

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






More information about the llvm-commits mailing list