[PATCH] D54137: AArch64: Fix invalid CCMP emission

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1554
 /// "not (and (not (and (setCC (cmp C)) (setCC (cmp D))))
 ///           (and (not (setCA (cmp A)) (not (setCB (cmp B))))))"
 /// and implemented as:
----------------
Maybe include a second step here, which explicitly reassociates this expression?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1564
+/// not (and (not (and (not A) (not B))) (not (and (not C) (not D)))), we
+/// can only implement 1 of the inner (not) operations, but not both!
 /// @{
----------------
Would it be possible to use an extra CCMP to "negate" a condition, using something like "ccmp xzr, xzr"?  I'm not sure it's actually a good idea, but it seems feasible.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1609
+/// \param MustBeFirst  Set to true if we must emit this sub-tree first because
+///                     we negate the condition to test.
 static bool canEmitConjunction(const SDValue Val, bool &CanNegate,
----------------
In other words, MustBeFirst is true when the outermost expression of the transformed tree is a "not", as opposed to an "and" or a "setcc"?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1611
 static bool canEmitConjunction(const SDValue Val, bool &CanNegate,
+                               bool &MustBeFirst, bool WillNegate,
                                unsigned Depth = 0) {
----------------
Missing documentation for WillNegate?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1642
 
     if (Opcode == ISD::OR) {
+      // For an OR expression we need to be able to naturally negate at least
----------------
IsOR?


Repository:
  rL LLVM

https://reviews.llvm.org/D54137