This is an archive of the discontinued LLVM Phabricator instance.

[X86] Use bit test instructions to optimize some logic atomic operations
ClosedPublic

Authored by pengfei on Feb 19 2022, 6:05 PM.

Diff Detail

Event Timeline

pengfei created this revision.Feb 19 2022, 6:05 PM
pengfei requested review of this revision.Feb 19 2022, 6:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2022, 6:05 PM
LuoYuanke added inline comments.Feb 19 2022, 9:49 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
30495

Use enum { BIT_SET, BIT_CLEAR, BIT_RESET} to replace {0, 1, 2}?

30504

Add comments that there is only 1 user checked in shouldExpandLogicAtomicRMWInIR()?

llvm/lib/Target/X86/X86InstrCompiler.td
848

Does LOG mean logic? Rename to ATOMIC_OP?
SCR means "set, clear reset"? Maybe add comments for it, so that it is well understood.

llvm/lib/Target/X86/X86IntrinsicsInfo.h
40 ↗(On Diff #410121)

It seems LOCK_BITTEST is not used?

pengfei updated this revision to Diff 410134.Feb 19 2022, 11:53 PM
pengfei marked an inline comment as done.

Address Yuanke's comments.

llvm/lib/Target/X86/X86ISelLowering.cpp
30495

There's no other user, so I just add a comment here.

30504

There is it at line 30466.

llvm/lib/Target/X86/X86IntrinsicsInfo.h
40 ↗(On Diff #410121)

Yes. Good catch!

craig.topper added inline comments.Feb 20 2022, 4:33 PM
llvm/include/llvm/IR/IntrinsicsX86.td
67

Probably better as int_x86_atomic_bitttest?

llvm/lib/Target/X86/X86ISelLowering.cpp
5450

Does something check natural alignment before we create the intrinsic?

5451

Does this need MOVolatile?

30509

Does AI->getContext() work?

30515

CreateBitCast -> createPointerCast

30515

Probably doesn't really matter, but you should pass the address space to getInt8PtrTy.

30517

ConstantInt::get(Type::getInt8Ty(Ctx), Imm) -> Builder.getInt8(Imm)

30518

Builder.getInt32(SCR)

llvm/lib/Target/X86/X86InstrCompiler.td
847

Why no SDNPMayLoad?

848

Can we use 3 separate intrinsics? I'm not sure this made up encoding is saving much.

pengfei updated this revision to Diff 410302.Feb 21 2022, 7:29 AM
pengfei marked 6 inline comments as done.

Address review comments. Thanks Craig!

llvm/lib/Target/X86/X86ISelLowering.cpp
5450

Yes, it's checked by atomicSizeSupported in AtomicExpandPass.cpp

5451

Maybe not. MOVolatile is usually set when isVolatile() return true in load and store instructions. We don't have such info in a target intrinsic.
But seems we never worried about it, and there're few place it is set in X86 code. So I guess X86 is fine with or without MOVolatile?

30515

I didn't see we pass address space when using CreatePointerCast, I guess the address space keep unchanged?

craig.topper added inline comments.Feb 21 2022, 10:18 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
5451

This information is used to create a MachineMemOperand. I think that MachineMemOperand needs to know this access can't undergo certain optimizations because it represents an atomic access. I don't think this interface can create an atomic MachineMemOperand, but it can create a volatile one.

pengfei added inline comments.Feb 21 2022, 5:48 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
5451

volatile is an memory attribute which is not atomic exclusive. https://godbolt.org/z/1o9Y7j5xe
We have ignored it on all existing target memory intrinsics, so I think we don't need any special handling here.

craig.topper added inline comments.Feb 21 2022, 6:00 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
5451

Do the LOCK_BTS/BTR/BTC show up in MIR with "monotonic"(or other atomic ordering) in their memory operand printing in MIR? If not you have a bug waiting to happen. I think "volatile" is stronger than any of the atomic orderings. So it works in place of them.

RISCV uses MOVolatile in getTgtMemIntrinsic for riscv_masked_atomicrmw*

craig.topper added inline comments.Feb 21 2022, 6:06 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
5451

The important thing is that the isUnordered() method on the MachineMemOperand must return false if the original atomicrmw was ordered.

pengfei added inline comments.Feb 21 2022, 6:37 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
5451

I see. Yeah, I have worry on the same thing. That's why I didn't use target intrinsic at beginning. I'm glad it solves my problems. Thanks Craig!

pengfei updated this revision to Diff 410429.Feb 21 2022, 6:44 PM

Add attribute MOVolatile.

This revision is now accepted and ready to land.Feb 28 2022, 2:46 PM
This revision was landed with ongoing or failed builds.Feb 28 2022, 6:35 PM
This revision was automatically updated to reflect the committed changes.