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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer 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;
----------------
aadg wrote:
> aadg wrote:
> > 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. 
> > 
> > 
> After thinking about it a bit more, testing the out of bound case for upper range is ... UB. But there are definitely some testcases missing, and those will be added.
> After thinking about it a bit more, testing the out of bound case for upper range is ... UB. 

Ah yes, of course. I was thinking it could inhibit this rewrite.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:1879
+      !isLegalArithImmed(cast<ConstantSDNode>(RHS)->getZExtValue())) {
+    SDValue TheLHS = isCMN(LHS, CC) ? LHS.getOperand(1) : LHS;
+
----------------
aadg wrote:
> 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
> I think it's best to have the example in terms of what we are trying to achieve at the ISA

Fair enough.

> Regarding TheLHS, it's a pattern often found in the code base and denotes what is the LHS (theLHS) 

Sure, but we have 2 variables `TheLHS` and `LHS` here, which could be a bit confusing. Anyway, as I said, I was nitpicking, so please ignore.


Repository:
  rL LLVM

https://reviews.llvm.org/D53067