[PATCH] D64768: [InstructionSimplify] Apply sext/trunc after pointer stripping

Mon Jul 15 18:02:43 PDT 2019

hliao added a comment.

In D64768#1586690 <https://reviews.llvm.org/D64768#1586690>, @jdoerfert wrote:

> LGTM, one request and a proposal to fix it slightly differently.
>
> In D64768#1586555 <https://reviews.llvm.org/D64768#1586555>, @hliao wrote:
>
> > In D64768#1586435 <https://reviews.llvm.org/D64768#1586435>, @jdoerfert wrote:
> >
> > > This is fine with me. Though, I'm unsure if this should not live in the `stripAndAccumulateConstantOffsets` method.
> > >
> > > The problem is that we have two bit-widths now, pointer before and after. Since we have to express offset in one of them, we have a choice.
> > >  I think we already guarantee that the offset is representable in the initial bit-width so that was what I choose. Question is, do we want
> > >  to change it to be in the resulting bit-width or not. Either way, we should add a comment to the stripAndAccumulateConstantOffsets method
> > >  explaining the situation.
> >
> >
> > As I mentioned, without this fix, there are crashes when compiling similar patterns. The issue is that LHS gets the original bit-width as the offset is 0 and RHS gets the one after stripping. Later, as the stripped pointers point the same object, instsimplify will create compare of the offset, that triggers the crash. We have to ensure offset after stripping has the same bit-width.
>
>
> I understand. That is what I tried to describe above. My question was: Should `stripAndAccumulateConstantOffsets` return the offset wrt. the bit-width of the input pointer or output pointer? I can see arguments for both. If you think this way is better you can go ahead and commit. If you think the other way around is better, we should move the conversion into `stripAndAccumulateConstantOffsets`. I personally tend towards the latter.
>
> Either way, we have to add a comment to the `stripAndAccumulateConstantOffsets` method explaining the choice!

I prefer the offset to output pointer as that's that API is designed for, strip any constant adjustments. If the final pointer has different bit-width, we'd better prepare that offset matching that pointer's integer width.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64768