Page MenuHomePhabricator

[SCEV] Index type usually is, but is not guaranteed to be, equal to the pointer bit width
AbandonedPublic

Authored by lebedev.ri on Oct 16 2020, 3:52 AM.

Details

Summary

As it is being noticed in D89456, and being pointed out
by @efriedma https://reviews.llvm.org/D89456#inline-831603
if we use narrower type to represent the ConstantRange of a pointer,
we will end up with a wrong result.

In datalayout, there's pointer bit width, and a index bit width.
Latter is, by default, equal to the pointer bit width, but it can be different.
And SCEV uses index bit width everywhere, where pointer bit width was obviously meant..

Diff Detail

Unit TestsFailed

TimeTest
440 mswindows > LLVM.tools/llvm-cov::warnings.h
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-cov.exe show C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/prevent_false_instantiations.covmapping -instr-profile C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov/Inputs/elf_binary_comdat.profdata -path-equivalence=/tmp,C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-cov\warnings.h -allow-empty -check-prefix=FAKE-FILE-STDOUT
250 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w64\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w64\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w64\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

Event Timeline

lebedev.ri created this revision.Oct 16 2020, 3:52 AM

I'm not sure I completely understand the history from https://reviews.llvm.org/D42123. But I think the idea is that for pointer-type SCEVs, we want to do SCEV arithmetic using the index type because the other bits aren't relevant: from my understanding, they don't participate in pointer arithmetic.

So I think the right solution is to leave this code alone. And maybe don't create SCEVPtrToInt expressions for ptrtoint operations on pointers where getIndexTypeSizeInBits() isn't equal to getTypeSizeInBits(), at least initially, since it's sort of tricky to reason about.

lebedev.ri added a comment.EditedOct 16 2020, 9:55 AM

Thank you for taking a look!

I'm not sure I completely understand the history from https://reviews.llvm.org/D42123. But I think the idea is that for pointer-type SCEVs, we want to do SCEV arithmetic using the index type because the other bits aren't relevant: from my understanding, they don't participate in pointer arithmetic.

So I think the right solution is to leave this code alone. And maybe don't create SCEVPtrToInt expressions for ptrtoint operations on pointers where getIndexTypeSizeInBits() isn't equal to getTypeSizeInBits(), at least initially, since it's sort of tricky to reason about.

I question sanity of that approach.
That being said, then perhaps D89456 simply should also pretend that the pointer's high bits don't exist if they don't matter for us?

Assuming we have properly typed SCEVs, we can pretend the high bits of the pointer don't exist in a pointer SCEV. We can't pretend they don't exist in an integer SCEV: that leads to miscompiles. This is why SCEVPtrToIntExpr, specifically, needs special handling.

My suggestion was just to skip creating SCEVPtrToInt was mostly so you don't have to write a bunch of tests for various operations on SCEVPtrToInt expressions involving pointers with a narrow index type. It's probably not hard to implement, just sort of tedious, and we don't have good testing infrastructure.

lebedev.ri abandoned this revision.Oct 16 2020, 11:44 AM

Okay, i'm fine with that.