This is an archive of the discontinued LLVM Phabricator instance.

AtomicExpand: Avoid some operations if the atomic is overaligned
ClosedPublic

Authored by arsenm on Sep 20 2022, 3:45 PM.

Details

Summary

Let some of the pointer bithacking fold away if we know the LSB are 0.

Diff Detail

Build Status
Buildable 188428

Event Timeline

arsenm created this revision.Sep 20 2022, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 3:45 PM
arsenm requested review of this revision.Sep 20 2022, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 3:45 PM
Herald added a subscriber: wdng. · View Herald Transcript
rampitec added inline comments.Sep 20 2022, 4:04 PM
llvm/lib/CodeGen/AtomicExpandPass.cpp
727

Should this also go under if? Looks like for an overaligned case it is a no-op. I can actually see shl by 0 in the produced IR.

arsenm added inline comments.Sep 20 2022, 4:05 PM
llvm/lib/CodeGen/AtomicExpandPass.cpp
727

Yes, but this was less effort than writing separate code to handle the mask shifting

rampitec accepted this revision.Sep 20 2022, 4:07 PM

LGTM

llvm/lib/CodeGen/AtomicExpandPass.cpp
727

Oh, I see. It is cleaned up by the D134324.

This revision is now accepted and ready to land.Sep 20 2022, 4:07 PM
jyknight added inline comments.Sep 22 2022, 7:58 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
709–715

Shouldn't this folding get handled by the IR folder already? Why do we need to special case it here?

arsenm added inline comments.Sep 22 2022, 7:59 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
709–715

I don't follow. This check switches the path to avoid creating the ptrtoint

jyknight added inline comments.Sep 22 2022, 11:52 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
709–715

What I meant is that when the alignment of Addr is high enough, this could theoretically be optimized away automatically, based on the known-bits computation of Addr causing PtrLSB to be folded to 0, and then AddrInt ending up with no uses.

However, that was a silly comment on my part: InstSimplifyFolder couldn't remove an instruction with no uses, even if it could optimize away everything else. So the ptrtoint would remain regardless. And, this pass comes late enough in the pass pipeline that we aren't going to clean that up after the fact, so we really do want a special-case like this. OK.

725

You lost the isLittleEndian case here. I think the best fix would be to minimize the amount of code in the conditional section.

Instead of setting PMV.ShiftAmt here, just set PtrLSB = ConstantInt::getNullValue(IntTy) in the else clause, and move the entire PMV.ShiftAmt computation into unconditional code -- letting the constant folder take care of things.

arsenm updated this revision to Diff 462339.Sep 22 2022, 3:42 PM
arsenm marked an inline comment as done.

address comments

jyknight accepted this revision.Sep 23 2022, 8:57 AM
jyknight added inline comments.
llvm/lib/CodeGen/AtomicExpandPass.cpp
733

JFTR: same comment as previous patch that added this code: don't need the if.

arsenm added inline comments.Sep 23 2022, 8:58 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
733

I was thinking this was a handy marker to remove unnecessary code whenever typed pointers are finally dropped

arsenm updated this revision to Diff 462517.Sep 23 2022, 9:24 AM

Remove opaque pointer check