Page MenuHomePhabricator

[BasicAA] Use index size instead of pointer size

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



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

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

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.

@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

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

aardappel added inline comments.Nov 4 2021, 9:44 AM


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


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.