[PATCH] D30416: [InstCombine] Redo reduceLoadOpStoreWidth in instcombine for bitfield store optimization.

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5719
+         iterator_range<BasicBlock::const_iterator>(Start, End))
+      if (Inst.mayWriteToMemory())
+        return true;
----------------
We don't have alias analysis in CGP at all, do we?
Maybe it would be better to pull this out somewhere else (late in the pipeline).


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5739
+  // Limit the maximum number of BasicBlocks to 3 to protect compile time.
+  const int MaxBB = 3;
+  SmallPtrSet<const BasicBlock *, 4> BBSet;
----------------
Oh, ok, now I see why Eli suggested MemorySSA.
There has to be a better way to do this. (Although I can't think of one at the moment.)


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5823
+  if (ActualModBits)
+    ActualModBits = ActualModBits - Mask.countLeadingZeros();
+}
----------------
Are you sure this does the right thing for xor?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5895
+  unsigned TBits = DL.getTypeStoreSizeInBits(StoreTy);
+  if (TBits != DL.getTypeStoreSizeInBits(StoreTy))
+    return false;
----------------
I don't think this is what you meant to check here. :-)


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5912
+  // LI should have the same address as SI.
+  if (LI->getOperand(0) != Ptr)
+    return false;
----------------
Do we want to look through bitcasts? It probably doesn't matter in practice, though.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1388
+                               "uglygep");
+    Align = DL.getABITypeAlignment(NewTy);
+  }
----------------
wmi wrote:
> efriedma wrote:
> > wmi wrote:
> > > efriedma wrote:
> > > > wmi wrote:
> > > > > efriedma wrote:
> > > > > > What does the ABI type alignment have to do with this?
> > > > > If StOffset is 0, the NewPtr is the same as Ptr and the alignment of original store can be reused. If StOffset is not 0, use the ABI type alignment. 
> > > > Yes, you're stating what the code does, but it still doesn't make any sense.  There isn't any reason to expect that adding an offset will increase the alignment of the pointer.
> > > I am not expecting the alignment will increase. I am worried that the original alignment will be overestimated if directly applied to the new store and caused undefine behavior.
> > > Suppose the original i32 store to address @a has 32 bits alignment. Now we will store an i16 to a.f2 which is at address "@a + 2B".   "@a + 2B" should only have 16bits alignment.
> > > Suppose the original i32 store to address @a has 32 bits alignment. Now we will store an i16 to a.f2 which is at address "@a + 2B". "@a + 2B" should only have 16bits alignment.
> > 
> > Suppose the original i32 store to address @a has 8 bits alignment. What is the alignment of "@a + 2B"?  (You need to compute the GCD of the offset and the original alignment.)
> You are right. 
> 
> class A { 
> public: 
> unsigned long f1:8;
> unsigned long f2:16;
> unsigned long f3:8;
> }; 
> A a; 
> 
> int foo () {  
>   a.f2 = 3;  
> } 
> 
> i16 has 16 bits natural alignment, but a.f2 only has 8 bits alignment here. 
Could you add a test-case like this?


Repository:
  rL LLVM

https://reviews.llvm.org/D30416