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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Your testcase has dead instructions (%conv).

This is missing a significant safety check: we have to make sure the memory isn't modified between the load and the store.

It's not clear that we want to perform this transformation so early in the pipeline... dealing with overlapping stores makes other optimizations more difficult.  Do you have performance numbers for this?



================
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);
----------------
This will crash if the value is too large (e.g. i128).  You can use APInt operations instead.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1388
+                               "uglygep");
+    Align = DL.getABITypeAlignment(NewTy);
+  }
----------------
What does the ABI type alignment have to do with this?


================
Comment at: test/Transforms/InstCombine/bitfield-store.ll:89
+; a4.f1 = n;
+; The bitfield store cannot be shrinked because the field is not 8/16/32 bits.
+; CHECK-LABEL: @test4(
----------------
I'm not following; I'm pretty sure we could shrink the store if we wanted to.  Or do you mean it isn't profitable to shrink it?


Repository:
  rL LLVM

https://reviews.llvm.org/D30416