Page MenuHomePhabricator

[IR][InstCombine] IntToPtr Produces Typeless Pointer To Byte
Needs ReviewPublic

Authored by lebedev.ri on Mar 22 2021, 3:17 PM.

Details

Summary

While ptrtoint can and should be treated transparently by the passes,
inttoptr is a black box. SCEV does not, should not, and will not model it.
But that means, given two identical, but separate inttoptr instructions,
SROA won't be able to tell that they have the same pointer value.
Likewise for pointers with the same integer base but different pointee types.

Currently, InstCombine already ensures that inttoptr/ptrtoint don't change
the integer width, but it is performed separately before/after.

Let's do the same for target pointee type for inttoptr.
I.e. if it's not an i8*, reconstruct inttoptr + emit bitcast.
And let's rely on EarlyCSE to be able to CSE them,
thus making them somewhat less of a hurdle for SROA/etc.

Notably, i don't think we should do the same for ptrtoint.

Now, this causes some test regressions.
I've fixed SROA issue (D99051), and one InstCombine issue
(p2i (?bitcast (ins (?bitcast (i2p Vec)), Scalar, Index)) --> ins Vec, (p2i Scalar), Index),
but there may be few more in tests, i'll comment on them.
I'm not sure off-hand about what to do with them.

Diff Detail

Unit TestsFailed

TimeTest
190 msx64 windows > LLVM.ExecutionEngine/MCJIT::cross-module-sm-pic-a.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\lli.exe -mtriple=x86_64-pc-windows-msvc-elf -jit-kind=mcjit -extra-module=C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT/Inputs/cross-module-b.ll -relocation-model=pic -code-model=small C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT\cross-module-sm-pic-a.ll > /dev/null
160 msx64 windows > LLVM.ExecutionEngine/MCJIT::multi-module-sm-pic-a.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\lli.exe -mtriple=x86_64-pc-windows-msvc-elf -jit-kind=mcjit -extra-module=C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT/Inputs/multi-module-b.ll -extra-module=C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT/Inputs/multi-module-c.ll -relocation-model=pic -code-model=small C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT\multi-module-sm-pic-a.ll > /dev/null
170 msx64 windows > LLVM.ExecutionEngine/MCJIT::stubs-sm-pic.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\lli.exe -mtriple=x86_64-pc-windows-msvc-elf -jit-kind=mcjit -disable-lazy-compilation=false -relocation-model=pic -code-model=small C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT\stubs-sm-pic.ll
150 msx64 windows > LLVM.ExecutionEngine/MCJIT::test-global-init-nonzero-sm-pic.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\lli.exe -mtriple=x86_64-pc-windows-msvc-elf -jit-kind=mcjit -relocation-model=pic -code-model=small C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT\test-global-init-nonzero-sm-pic.ll > /dev/null
150 msx64 windows > LLVM.ExecutionEngine/MCJIT::test-ptr-reloc-sm-pic.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\lli.exe -mtriple=x86_64-pc-windows-msvc-elf -jit-kind=mcjit -O0 -relocation-model=pic -code-model=small C:\ws\w16-1\llvm-project\premerge-checks\llvm\test\ExecutionEngine\MCJIT\test-ptr-reloc-sm-pic.ll

Event Timeline

lebedev.ri created this revision.Mar 22 2021, 3:17 PM
lebedev.ri requested review of this revision.Mar 22 2021, 3:17 PM
lebedev.ri edited the summary of this revision. (Show Details)Mar 22 2021, 3:22 PM
lebedev.ri added inline comments.
llvm/test/Transforms/InstCombine/indexed-gep-compares.ll
35–51

Regression. Not looking through bitcast?

105–122

Regression. Not looking through bitcast?

189–213

Regression. Not looking through bitcast?

llvm/test/Transforms/InstCombine/intptr3.ll
14–16

Regression.

llvm/test/Transforms/InstCombine/intptr7.ll
4–27

Regression. Not looking through bitcast?

52–75

Regression. Not looking through bitcast?

nikic added a reviewer: nikic.Mar 22 2021, 3:35 PM
ruiling added inline comments.Mar 22 2021, 11:43 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
278–280

If we want to disable int2ptr + bitcast unconditionally, then why not directly make it in CastInst::isEliminableCastPair()?

1949–1953

The idea here sounds good to me, but seems causing some new problems for the regression tests. I am not sure the whether the regressions are easy to fix?

lebedev.ri marked an inline comment as done.

Also update a few clang tests.

Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 3:45 AM

The pointee type in LLVM doesn't really matter. It's even supposed to disappear one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.
So, I don't understand the motivation for this patch. It doesn't solve the root cause of the problem (which one btw?).

The pointee type in LLVM doesn't really matter. It's even supposed to disappear one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.

Yep. That will be indeed a great to see.

So, I don't understand the motivation for this patch. It doesn't solve the root cause of the problem (which one btw?).

It is indeed temporary until Opaque pointers are here.
The problem has been stated last time in D99051 by @ruiling:
https://godbolt.org/z/x7E1EjWvv, i.e. given the same integer,
there can be any number of pointers inttoptr'd from it,
and passes won't be able to tell that they are identical.

@dblaikie @t.p.northover can anyone comment on the Opaque Pointers progress? Is there a checklist somewhere?

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
278–280

Because technically that is still legal, much like changing integer types of the inttoptr/ptrtoint,
which also aren't prohibited in isEliminableCastPair().

1949–1953

FWIW all the problems are temporary until LLVM IR is migrated to typeless pointer types.

The pointee type in LLVM doesn't really matter. It's even supposed to disappear one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.

Yep. That will be indeed a great to see.

So, I don't understand the motivation for this patch. It doesn't solve the root cause of the problem (which one btw?).

It is indeed temporary until Opaque pointers are here.
The problem has been stated last time in D99051 by @ruiling:
https://godbolt.org/z/x7E1EjWvv, i.e. given the same integer,
there can be any number of pointers inttoptr'd from it,
and passes won't be able to tell that they are identical.

@dblaikie @t.p.northover can anyone comment on the Opaque Pointers progress? Is there a checklist somewhere?

no checklist, unfortunately - myself, @t.p.northover, @jyknight, and @arsenm have all done bits and pieces of work on it lately.

I think we've got most of the big IR changes (adding explicit types where they'll be needed when they're no longer carried on the type of pointer parameters) - @arsenm's D98146 is another piece in that area, hopefully near the last I think.

After all that's in place, the next step I think would be to introduce the typeless pointer, support it as an operand to these various operations - and then try producing it as a result of instructions too. But I'm probably missing a bunch of important steps we'll find are necessary...

The pointee type in LLVM doesn't really matter. It's even supposed to disappear one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.

Yep. That will be indeed a great to see.

So, I don't understand the motivation for this patch. It doesn't solve the root cause of the problem (which one btw?).

It is indeed temporary until Opaque pointers are here.
The problem has been stated last time in D99051 by @ruiling:
https://godbolt.org/z/x7E1EjWvv, i.e. given the same integer,
there can be any number of pointers inttoptr'd from it,
and passes won't be able to tell that they are identical.

@dblaikie @t.p.northover can anyone comment on the Opaque Pointers progress? Is there a checklist somewhere?

no checklist, unfortunately - myself, @t.p.northover, @jyknight, and @arsenm have all done bits and pieces of work on it lately.

I think we've got most of the big IR changes (adding explicit types where they'll be needed when they're no longer carried on the type of pointer parameters) - @arsenm's D98146 is another piece in that area, hopefully near the last I think.

After all that's in place, the next step I think would be to introduce the typeless pointer, support it as an operand to these various operations - and then try producing it as a result of instructions too. But I'm probably missing a bunch of important steps we'll find are necessary...

Do you have an ETA for when the switch will happen? Just to inform us where we should proceed with temp fixes like this one or we should just wait.

The pointee type in LLVM doesn't really matter. It's even supposed to disappear one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.

Yep. That will be indeed a great to see.

So, I don't understand the motivation for this patch. It doesn't solve the root cause of the problem (which one btw?).

It is indeed temporary until Opaque pointers are here.
The problem has been stated last time in D99051 by @ruiling:
https://godbolt.org/z/x7E1EjWvv, i.e. given the same integer,
there can be any number of pointers inttoptr'd from it,
and passes won't be able to tell that they are identical.

@dblaikie @t.p.northover can anyone comment on the Opaque Pointers progress? Is there a checklist somewhere?

no checklist, unfortunately - myself, @t.p.northover, @jyknight, and @arsenm have all done bits and pieces of work on it lately.

I think we've got most of the big IR changes (adding explicit types where they'll be needed when they're no longer carried on the type of pointer parameters) - @arsenm's D98146 is another piece in that area, hopefully near the last I think.

After all that's in place, the next step I think would be to introduce the typeless pointer, support it as an operand to these various operations - and then try producing it as a result of instructions too. But I'm probably missing a bunch of important steps we'll find are necessary...

Do you have an ETA for when the switch will happen? Just to inform us where we should proceed with temp fixes like this one or we should just wait.

No, sorry I don't - I ran out of steam after the initial work, and haven't been able to get back into it. A few folks have picked up my slack in the last year or two & made some incremental progress.

It'd be good to tag any workarounds somehow (I don't know how, exactly) to be sure they're cleaned up as things are sorted out.

Long and the short of it: If these bugs matter to you, probably not worth waiting for the general fix (but more help would be appreciated if you wanted to work on the long term solution) & workarounds are probably reasonable.

No, sorry I don't - I ran out of steam after the initial work, and haven't been able to get back into it. A few folks have picked up my slack in the last year or two & made some incremental progress.

It'd be good to tag any workarounds somehow (I don't know how, exactly) to be sure they're cleaned up as things are sorted out.

Long and the short of it: If these bugs matter to you, probably not worth waiting for the general fix (but more help would be appreciated if you wanted to work on the long term solution) & workarounds are probably reasonable.

Sounds we still need a long way to get there? Do we think the patch acceptable as a short-term solution?

No, sorry I don't - I ran out of steam after the initial work, and haven't been able to get back into it. A few folks have picked up my slack in the last year or two & made some incremental progress.

It'd be good to tag any workarounds somehow (I don't know how, exactly) to be sure they're cleaned up as things are sorted out.

Long and the short of it: If these bugs matter to you, probably not worth waiting for the general fix (but more help would be appreciated if you wanted to work on the long term solution) & workarounds are probably reasonable.

Sounds we still need a long way to get there? Do we think the patch acceptable as a short-term solution?

Guess it's a tradeoff. Doesn't look like a load of code/complexity. - but equally, does it provide much value? Or is it enough to know it can eventually be addressed in a more general manner?

Sounds we still need a long way to get there? Do we think the patch acceptable as a short-term solution?

Guess it's a tradeoff. Doesn't look like a load of code/complexity. - but equally, does it provide much value? Or is it enough to know it can eventually be addressed in a more general manner?

I don't have an answer for the second question, but this change would make later pass like ScalarEvolution can know the two pointers from the same integer by inttoptr are identical. Then we can merge possible consecutive memory access based on such pointers, we observed about 2% overall performance improvement for a critical workload.

Given that it gives a decent speedup for some important workload, and provided it doesn't regress others, I think this should go in then.
It's easy to revert this once opaque pointers arrive.

nikic added a comment.Mar 25 2021, 6:00 AM

Given that it gives a decent speedup for some important workload, and provided it doesn't regress others [...]

I think that second point is going to need some evidence (at least in the form of transform stats I guess). D96881 is another recent attempt to work around pointer types, and it looks like that one did regress others. This patch already shows regressions inside InstCombine itself, so that's not a great sign.

Then what do you think of my initial change for the issue (D99051) considering it does not show regression in any existing test? @nikic @nlopes It has a test showing the initial problem I want to solve. @lebedev.ri helped writing this patch to solve the problem more completely. But I guess D99051 is enough for the initial problem, our frontend would less likely generating two pointers pointing to different types through inttoptr from the same integer.

foad added a subscriber: foad.Apr 7 2021, 1:55 AM