[PATCH] D53067: [AArch64] Swap comparison operands if that enables some folding.

[Arnaud A. de Grandmaison via Phabricator via llvm-commits]

aadg added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1802
+      EVT VT = Op.getValueType();
+      if ((VT == MVT::i32 && Shift <= 31) || (VT == MVT::i64 && Shift <= 63))
+        return 1;
----------------
SjoerdMeijer wrote:
> Had a quick first look, so sorry for some question about testing.... If the shift amount is in the range [0,31] and [0,64], are the edge cases tested (also values just outside this range)? Also, can `Shift` be negative here?
> 
> 
The ARMARM definitely states Shift has to be in [0:31] (32bits variant) or [0:63] (64 bits variant), so it can not be negative.
You however point rightfully that testcases for the out-of-range cases for the upper bound are missing --- I'll add them right away. 




================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1879
+      !isLegalArithImmed(cast<ConstantSDNode>(RHS)->getZExtValue())) {
+    SDValue TheLHS = isCMN(LHS, CC) ? LHS.getOperand(1) : LHS;
+
----------------
SjoerdMeijer wrote:
> Nit, feel free to ignore: perhaps the example can be given in terms of ISD nodes because that's what we're transforming here. And then `TheLHS` can perhaps be given a more meaningful name.
I think it's best to have the example in terms of what we are trying to achieve at the ISA level as this gives the reader the "why" we want to do it. The how we actually do it is what the code does.
Regarding `TheLHS`, it's a pattern often found in the code base and denotes what is the LHS (`theLHS`) under consideration at this point in the code, as it might differ from a high level LHS. I'm open to a better naming though if you have suggestions


Repository:
  rL LLVM

https://reviews.llvm.org/D53067