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

[Wei Mi via Phabricator via llvm-commits]

wmi 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);
----------------
efriedma wrote:
> 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.
Ah, I had a mental block here.  I was thinking MaskLeadOnes has to be APInt if I use APInt::countLeadingOnes and APInt doesn't support % operator.

It is better to APInt. I will fix it.


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


================
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(
----------------
efriedma wrote:
> wmi wrote:
> > efriedma wrote:
> > > 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?
> > There is a correctness issue. If we shrink the store and generate store like below:
> > 
> >   %t0 = trunc i32 %n to i13
> >   store i13 %t0, i13* bitcast (%class.A4* @a4 to i13*), align 8
> >   ret void
> > 
> > llvm will generate code:
> >         andl    $8191, %edi             # imm = 0x1FFF
> >         movw    %di, a4(%rip)
> >         retq
> > 
> > and a4.f2 will be overwritten.
> You could slightly tweak your optimization so it doesn't run into that problem: instead of truncating and storing MaskedVal, just truncate+store Val.
Sorry I don't get the point. Are you suggesting the following?

  %bf.set = or i16 %bf.clear3, %bf.value
  %bf.set.truncate = trunc %bf.set i16 to i13
  store i13 %bf.set.trunc, i13* bitcast (%class.A4* @a4 to i13*), align 8

llvm will still generate the same code:

  andl    $8191, %edi             # imm = 0x1FFF
  movw    %di, a4(%rip)


Repository:
  rL LLVM

https://reviews.llvm.org/D30416