Page MenuHomePhabricator

[BasicAA] Use index size instead of pointer size
ClosedPublic

Authored by nikic on Oct 23 2021, 2:38 PM.

Details

Summary

When accumulating the GEP offset in BasicAA, we should use the pointer index size rather than the pointer size.

Diff Detail

Event Timeline

nikic created this revision.Oct 23 2021, 2:38 PM
nikic requested review of this revision.Oct 23 2021, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2021, 2:38 PM
reames added inline comments.Oct 25 2021, 10:33 AM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
552

The use of maximum pointer/index size seems suspect here. Shouldn't we be working in the bitwidth of a single AS at all times?

nikic added inline comments.Oct 25 2021, 12:14 PM
llvm/lib/Analysis/BasicAliasAnalysis.cpp
552

GEP decomposition looks through address space casts, so the index size can change. It does mostly work with the index size of a single AS and then sext up to the maximum size, though the precise way it does so is not quite correct in some respects (wrt sext/mul commutativity and scale adjustment -- fixing this needs some more preliminary work though).

It would certainly make things easier if we could get away with not looking through address space casts, but I'm not sure that's viable.

nikic added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
509

@aardappel @tlively Is it okay to check the address space zero pointer size here? I don't really want to retain a separate getMaxPointerSize() method for only this usage, which looks like a bit of a hack anyway.

tlively added inline comments.Nov 2 2021, 1:43 PM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
509

Yes, AS 0 is the correct one to check here.

aardappel added inline comments.Nov 4 2021, 9:44 AM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
509

LGTM

nikic added a reviewer: nlopes.Sat, Nov 6, 8:41 AM
arichardson accepted this revision.Sat, Nov 6, 4:19 PM

Thanks, this LGTM and should hopefully avoid some unnecessary extends to 128 bits for CHERI.

I think it would be nice if we could avoid the use of the max pointer/index size here but that is an unrelated change that can be looked into later.

This revision is now accepted and ready to land.Sat, Nov 6, 4:19 PM
nlopes accepted this revision.Sun, Nov 7, 8:13 AM

LGTM

This revision was landed with ongoing or failed builds.Sun, Nov 7, 9:56 AM
This revision was automatically updated to reflect the committed changes.