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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1331
+  // will handle patterns like either 0..01..1 or 1..10..01..1
+  uint64_t Mask = Cst->getValue().getSExtValue();
+  unsigned MaskLeadOnes = countLeadingOnes(Mask);
----------------
wmi wrote:
> efriedma wrote:
> > This will crash if the value is too large (e.g. i128).  You can use APInt operations instead.
> bitfields wider than 32 bits are uncommon, so how about adding a limit of TBits <= 64?
It's not really that hard to use APInt operations... you just have to use APInt::countLeadingOnes/etc. rather than the int64_t versions.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1388
+                               "uglygep");
+    Align = DL.getABITypeAlignment(NewTy);
+  }
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D30416