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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
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(
----------------
wmi wrote:
> efriedma wrote:
> > wmi wrote:
> > > 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)
> > Oh, sorry, this isn't a good example; I mixed up the fields.  But consider:
> > 
> > ```
> > ; class ATest {
> > ;   unsigned long f1:13;
> > ;   unsigned long f2:3;
> > ; } atest;
> > ; atest.f2 = n;
> > ```
> > 
> > You could shrink the store here (trunc to i8).
> Ah, I see what you mean. In your case, we can shrink the store, but cannot remove the original load and bit operations doing the mask. I can add the shrink but I am not sure whether it is better than without the shrink. 
It's a substantial improvement if you're transforming from an illegal type to a legal type.  (I've been dealing with trying to optimize an i24 bitfield recently; see, for example, test/CodeGen/ARM/illegal-bitfield-loadstore.ll.)  In other cases, you're right, it's not obviously profitable.


Repository:
  rL LLVM

https://reviews.llvm.org/D30416