[PATCH] D116375: [X86] Use bit test instructions to optimize some logic atomic operations

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30894
+          continue;
+        if (C2 || UI->getOpcode() != ISD::AND)
+          report_fatal_error("Atomic result must be used by one AND");
----------------
craig.topper wrote:
> Verify that operand 1 is a constant too.
This is not used to check constantency but to check the code is entered once only (C2 hasn't been assigned to a constant value).
We check the constantency through `cast<ConstantSDNode>`


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30895
+        if (C2 || UI->getOpcode() != ISD::AND)
+          report_fatal_error("Atomic result must be used by one AND");
+        C2 = cast<ConstantSDNode>(UI->getOperand(1));
----------------
craig.topper wrote:
> My big concern is that we rely on the checks done in IR holding until SelectionDAG. If that fails and a user hits this fatal error, how would they know how to fix their code?
> 
> Did you give any thought about converting the patterns into a target specific intrinsic in IR? That would probably need to be a new hook in the AtomicExpandPass or a separate X86 pass.
> 
> @rksimon or @spatel what are your thoughts?
> Did you give any thought about converting the patterns into a target specific intrinsic in IR? That would probably need to be a new hook in the AtomicExpandPass or a separate X86 pass.

I'm not sure if we can simply use target specific intrinsic here. Atomic instruction has attributes like ordering that need special handling.

> My big concern is that we rely on the checks done in IR holding until SelectionDAG. If that fails and a user hits this fatal error, how would they know how to fix their code?

We mainly check 3 aspects here:
1. One use, constantency and consistency of the constants.
  - I don't think they will be changed given there are no room to optimize them.
2. AND operation.
  - It won't be changed either. The reason is the same. But we might have chance to roll back the AND if it changed.
3. AND operation been splitted to a different BB.
  - There may be chance to happen in reality. We can always hoise it by a separate pass before SelectionDAG. I'd like to leave it till it happens.

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116375/new/

https://reviews.llvm.org/D116375