Page MenuHomePhabricator

[InstCombine] combineLoadToOperationType(): don't fold int<->ptr cast into load
ClosedPublic

Authored by lebedev.ri on Oct 7 2020, 9:53 AM.

Details

Summary

And another step towards transforms not introducing inttoptr and/or
ptrtoint casts that weren't there already.

As we've been establishing (see D88788/D88789), if there is a int<->ptr cast,
it basically must stay as-is, we can't do much with it.

I've looked, and the most source of new such casts being introduces,
as far as i can tell, is this transform, which, ironically,
tries to reduce count of casts..

On vanilla llvm test-suite + RawSpeed, @ -O3, this results in
-33.58% less IntToPtrs (19014 -> 12629)
and +76.20% more PtrToInts (18589 -> 32753),
which is an increase of +20.69% in total.

However just on RawSpeed, where i know there are basically
none IntToPtr in the original source code,
this results in -99.27% less IntToPtrs (2724 -> 20)
and +82.92% more PtrToInts (4513 -> 8255).
which is again an increase of 14.34% in total.

To me this does seem like the step in the right direction,
we end up with strictly less IntToPtr, but strictly more PtrToInt,
which seems like a reasonable trade-off.

I'm not presently sure what preparatory fixes are needed before this though.

(Eventually, CastInst::isNoopCast()/CastInst::isEliminableCastPair
should be taught about this, yes)

Diff Detail

Unit TestsFailed

TimeTest
300 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w16c2-1\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w16c2-1\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 7 2020, 9:53 AM
lebedev.ri requested review of this revision.Oct 7 2020, 9:53 AM
lebedev.ri updated this revision to Diff 297217.Oct 9 2020, 6:14 AM

Rebased, NFC.

Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2020, 6:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lebedev.ri edited the summary of this revision. (Show Details)Oct 9 2020, 10:05 AM
nlopes added inline comments.Oct 11 2020, 8:16 AM
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
563

What about fixing isNoopCast() directly?
Is the impact too bad/large?

lebedev.ri added inline comments.Oct 11 2020, 8:21 AM
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
563

I don't recall, actually. That is the final step, yes,
but i think we can get most of the way there with small incremental changes.

lebedev.ri added inline comments.Oct 11 2020, 9:01 AM
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
563

(what i mean is, if "let's just do it" is the review feedback, then i can of course do it all at once..)

nikic accepted this revision.Oct 11 2020, 9:31 AM

LGTM

Looking through other uses of isNoopCast(), I don't think it makes sense to push this change into it, as many other usages do need it to work with ptrtoint/inttoptr (some of them using it specifically for them). The comment above the function indicates that "no-op" is to be understood as "generates no code" here. Possibly it could do with a rename.

This revision is now accepted and ready to land.Oct 11 2020, 9:31 AM
nikic added inline comments.Oct 11 2020, 9:32 AM
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
560

You might want to be a tad more explicit here, e.g. add "because they have different provenance" or so.

LGTM

@nlopes does this look good to you?

Looking through other uses of isNoopCast(), I don't think it makes sense to push this change into it, as many other usages do need it to work with ptrtoint/inttoptr (some of them using it specifically for them). The comment above the function indicates that "no-op" is to be understood as "generates no code" here. Possibly it could do with a rename.

I think i don't agree with you there.
I agree with @nlopes, the end goal will be to basically disallow fusing of inttoptr/ptrtoint into loads,
disallow dropping inttoptr-of-ptrtoint/ptrtoint-of-inttoptr, etc.
And all that eventually boils down to updating CastInst::isNoopCast()/CastInst::isEliminableCastPair().

nlopes accepted this revision.Oct 11 2020, 10:03 AM

LGTM

@nlopes does this look good to you?

Looking through other uses of isNoopCast(), I don't think it makes sense to push this change into it, as many other usages do need it to work with ptrtoint/inttoptr (some of them using it specifically for them). The comment above the function indicates that "no-op" is to be understood as "generates no code" here. Possibly it could do with a rename.

I think i don't agree with you there.
I agree with @nlopes, the end goal will be to basically disallow fusing of inttoptr/ptrtoint into loads,
disallow dropping inttoptr-of-ptrtoint/ptrtoint-of-inttoptr, etc.
And all that eventually boils down to updating CastInst::isNoopCast()/CastInst::isEliminableCastPair().

I'm ok with this change, and I agree it's a good step forwards. Thanks!
My only concern was that it seems this code will need further changes going forward (maybe even revert this change?) and I wouldn't want us to forget to revisit this code if/when needed.

@nlopes @nikic thank you for the review!

LGTM

@nlopes does this look good to you?

Looking through other uses of isNoopCast(), I don't think it makes sense to push this change into it, as many other usages do need it to work with ptrtoint/inttoptr (some of them using it specifically for them). The comment above the function indicates that "no-op" is to be understood as "generates no code" here. Possibly it could do with a rename.

I think i don't agree with you there.
I agree with @nlopes, the end goal will be to basically disallow fusing of inttoptr/ptrtoint into loads,
disallow dropping inttoptr-of-ptrtoint/ptrtoint-of-inttoptr, etc.
And all that eventually boils down to updating CastInst::isNoopCast()/CastInst::isEliminableCastPair().

I'm ok with this change, and I agree it's a good step forwards. Thanks!
My only concern was that it seems this code will need further changes going forward (maybe even revert this change?) and I wouldn't want us to forget to revisit this code if/when needed.

Once those methods are fixed, sure, the uses will need dead code cleanup.

Looking through other uses of isNoopCast(), I don't think it makes sense to push this change into it, as many other usages do need it to work with ptrtoint/inttoptr (some of them using it specifically for them). The comment above the function indicates that "no-op" is to be understood as "generates no code" here. Possibly it could do with a rename.

I think i don't agree with you there.
I agree with @nlopes, the end goal will be to basically disallow fusing of inttoptr/ptrtoint into loads,
disallow dropping inttoptr-of-ptrtoint/ptrtoint-of-inttoptr, etc.
And all that eventually boils down to updating CastInst::isNoopCast()/CastInst::isEliminableCastPair().

I agree with the general goal -- my point here is that changing isNoopCast() may not be the way to achieve that, because at least some of the current usages do need to include ptr/int casts, and can include them safely (not all usages result in type punning).

In fact, we already have a way to write "isNoopCast() without ptrtoint or inttoptr": isa<BitCast>. This didn't quite click before.... You might want to replace CI->isNoopCast(DL) && LI.getType()->isPtrOrPtrVectorTy() == CI->getDestTy()->isPtrOrPtrVectorTy()) here with just isa<BitCast>(CI).

+1 for this patch!