This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify][InstCombine] Fold ptrtoint(gep i8 null, x) -> x
AbandonedPublic

Authored by arichardson on Sep 22 2021, 6:58 AM.

Details

Summary

This commit is the InstSimplify follow-up to the previous constant-folding
change that enables noticeable optimizations for CHERI-enabled targets.

We could fold all ptrtoint(inbounds GEP) to zero here since that is the
only valid offset for an inbounds GEP. If the offset is not zero, that GEP
is poison so returning 0 is valid (https://alive2.llvm.org/ce/z/Gzb5iH).
However, Clang currentlygenerates inbounds GEPs on NULL for hand-written
offsetof expressions, so this risks miscompilation.

FIXME: is this transformation also valid for non-integral pointers?

Depends on D110245

Diff Detail

Event Timeline

arichardson created this revision.Sep 22 2021, 6:58 AM
arichardson requested review of this revision.Sep 22 2021, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2021, 6:58 AM
  • Use Q.DL.getTypeAllocSize instead of Q.DL.getTypeSizeInBits
  • Drop unnecessary code and comments
lebedev.ri requested changes to this revision.Sep 23 2021, 3:03 AM
lebedev.ri added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4673
4679

Doesn't this essentially call ->getType() on a pointer?
You can't do that. Perhaps you want getSourceElementType()?

4680

What if it's a constant?

4685

This is surely broken

This revision now requires changes to proceed.Sep 23 2021, 3:03 AM
  • Use isNonIntegralAddressSpace(GEP->getAddressSpace())
arichardson marked an inline comment as done.Sep 23 2021, 3:08 AM
arichardson added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4680

In that case it should have been handled by D110245? Will add a comment.

4685

In what way? It's here to ensure that the following isn't incorrectly folded to %val:

; We can't fold non-i8 GEPs in InstSimplify since that would require adding new arithmetic.
; TODO: handle this case in InstCombine
define i64 @fold_ptrtoint_nullgep_i32_variable(i64 %val) {
  %ptr = getelementptr i32, i32 addrspace(1)* null, i64 %val
  %ret = ptrtoint i32 addrspace(1)* %ptr to i64
  ret i64 %ret
}
lebedev.ri added inline comments.Sep 23 2021, 3:11 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4685

Why 8 and not 42?

  • fix getTypeAllocSize
arichardson marked an inline comment as done.Sep 23 2021, 3:21 AM
lebedev.ri added inline comments.Sep 23 2021, 3:31 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4682
arichardson added inline comments.Sep 23 2021, 4:25 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4682

Sorry, I don't quite understand what this link is showing. The base is not null and there is more than one GEP index so this transformation won't happen.

4685

Good catch, that was a bad copy-paste error from TypeSizeInBits().

If I understand the GEP logic correctly, then TypeAllocSize == 1 implies there is no multiplier and we can therefore return the index unchanged.

lebedev.ri added inline comments.Sep 23 2021, 4:30 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4682

I'm saying that the precondition you want is that the only non-zero index must be the last one (+ stride check),
not that there must only be a single index.

arichardson added inline comments.Sep 23 2021, 4:31 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4682

Ah yes of course. Will update the patch. Right now that is only handled by the InstCombine follow-up (D110247)

  • Handle multiple index as long as only the last one is non-zero
lebedev.ri added inline comments.Sep 23 2021, 8:20 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4681–4685

Can this be something closer to this?
I'm having trouble parsing the current one..

  • Try to increase readability by using llvm::all_of
arichardson marked an inline comment as done.Sep 23 2021, 8:49 AM
lebedev.ri accepted this revision.Sep 23 2021, 8:55 AM

LG with nits.
No idea about non-integral pointers.

llvm/lib/Analysis/InstructionSimplify.cpp
4684–4685

Then you might aswell just use the one from C++ STL taking separate begin/end

4687

The LastIndexOp->getType() == Ty check can be done earlier, before all_of check
(or just inline OtherIndicesAllZero variable?)

4688–4691

Again, this seems like a cheaper check that should be done before all_of

This revision is now accepted and ready to land.Sep 23 2021, 8:55 AM
arichardson marked 3 inline comments as done.
  • simplify and reorder to do cheap check first.
lebedev.ri accepted this revision.Sep 23 2021, 10:32 AM

I'm not convinced that it makes sense to add this to InstSimplify. This is extra complexity to cover a really narrow subset of what InstCombine will handle anyway. This already seems like quite the edge case fold -- is there some reason to believe that it is critical to handle the "i8 with variable index" special case in InstSimplify?

I'm not convinced that it makes sense to add this to InstSimplify. This is extra complexity to cover a really narrow subset of what InstCombine will handle anyway. This already seems like quite the edge case fold -- is there some reason to believe that it is critical to handle the "i8 with variable index" special case in InstSimplify?

I think you are right, for us it seems like the most important cases will be handled by constant folding and deferring the rest to InstCombine instead of InstSimplify should address the rest. Will drop this and just focus on the InstCombine fold.

arichardson abandoned this revision.Sep 24 2021, 1:00 AM