[PATCH] D30416: [InstCombine] Redo reduceLoadOpStoreWidth in instcombine for bitfield store optimization.
Wei Mi via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 27 22:02:49 PST 2017
wmi added inline comments.
================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1388
+ "uglygep");
+ Align = DL.getABITypeAlignment(NewTy);
+ }
----------------
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.
================
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:
> > > 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.
Repository:
rL LLVM
https://reviews.llvm.org/D30416
More information about the llvm-commits
mailing list