[PATCH] D149893: Rewrite LSV to handle longer chains.

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D149893#4387260 <https://reviews.llvm.org/D149893#4387260>, @bjope wrote:

> In D149893#4387256 <https://reviews.llvm.org/D149893#4387256>, @jlebar wrote:
>
>> @bjope sorry for the breakage.
>>
>> I am fine with this.  Would you be willing to send a patch?
>
> Ok, great. I'll put something up for review.

I've been hesitating a bit.
Mainly because the idea to use `DL.getPrefTypeAlign(Type::getInt32Ty(F.getContext()));` is pretty ugly as well (for example not considering address spaces etc).

And I've realized that if I add "S16" to the data layout for my target, then `getOrEnforceKnownAlignment` won't increase the alignment to 4 when the origin is an alloca. So that would avoid the dynamic stack align that I've seen.

However, we could still get an increased align for non-alloca accesses. And here I got a bit confused. The code comments say "If we're loading/storing from an alloca, align it if possible.", but then the code only compares the address space with the alloca address space. Now we might end up changing the align of global variables as well.
I think that there should be a more fine grained check to look at the ptr operand to see if we actually access an alloca, if that code comment should be fulfilled.
Something like this:

  // If we're loading/storing from an alloca, align it if possible.
  Value *PtrOperand = getLoadStorePointerOperand(C[CBegin].Inst);
  bool IsAllocaAccess = isa<AllocaInst>(PtrOperand->stripPointerCasts());
  if (IsAllocaAccess && ...

Or is it the code comment that is misleading?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149893