[PATCH] D68852: [Attributor] Pointer privatization attribute (argument promotion)

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4697
+    } else {
+      ReplacementValues.push_back(new LoadInst(PrivType, Base, "", IP));
+    }
----------------
efriedma wrote:
> jdoerfert wrote:
> > efriedma wrote:
> > > jdoerfert wrote:
> > > > efriedma wrote:
> > > > > jdoerfert wrote:
> > > > > > efriedma wrote:
> > > > > > > No alignment set on loads?
> > > > > > I'll add the alignment for the alloca below based on the alignment of the pointer it replaces. The loads and stores will be annotated by the next run of the Attributor automatically.
> > > > > > 
> > > > > > We can also consider not emitting it if we can prove it is not needed, though that will not always be the case and it will require an analysis we do not yet have (something like AAAccessTracker).
> > > > > > 
> > > > > > Finally, SROA should, in the good case, eliminate the alloca completely.
> > > > > > The loads and stores will be annotated by the next run of the Attributor automatically.
> > > > > 
> > > > > The default alignment of a load is the alignment of the load's type, as computed by the datalayout.  This might be too high, depending on the pointer.
> > > > > The default alignment of a load is the alignment of the load's type, as computed by the datalayout.
> > > > 
> > > > Sure.
> > > > 
> > > > 
> > > > > This might be too high, depending on the pointer.
> > > > 
> > > > How could that be? We create the pointer with a proper type (the alloca) below. Shouldn't the alloca take the default alignment into account when the memory is allocated?
> > > If you create an alloca, then load/store to that pointer, the default alignments will work, yes.  But that isn't what's happening here, is it?  The alloca is in the callee, and this load is in the caller.
> > With this patch we always have an alloca or an argument with some pointer to (struct) type which we only access through proper gep addressing. I don't think this can create an alignment issue.
> > 
> > I get that the alloca needs to be aligned with a higher value if the pointer was marked as such, but I already said that will be fixed.
> Pointers can be misaligned, generally.  For example:
> 
> ```
> define void @f() {
> entry:
>   %a = alloca i32, align 1
>   call void @g(i32* %a)
>   ret void
> }
> 
> define internal void @g(i32* %a) {
>   %aa = load i32, i32* %a, align 1
>   call void @z(aa)
> }
> 
> declare void @z(i32)
> ```
> 
> As far as I can tell, your patch will introduce a misaligned load into `@f()`.
> 
> (C generally provides additional guarantees based on the pointee type of a pointer, but there isn't any corresponding rule for IR pointers.)
I finally understand your concern, sorry that it took so long.

I played around a bit to see what we currently do and I found this interesting: https://godbolt.org/z/2q_oqH
We basically align the alloca naturally at some point.

I would for now just set the alignment to 1 and add a TODO. For these loads, the Attributor can find a better alignment in the next run anyway and this allows me to not amend this patch too much. The TODO will explain the situation and we can work on a better solution from then. Maybe, if it is very simple, I'll directly use the AAAlign logic to get a lower bound instead. Long story short, I'll make sure these loads are properly aligned and we test for this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68852/new/

https://reviews.llvm.org/D68852