[PATCH] D139618: [AArch64][SVE][Fixed length] Fix div miscompile

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:22630
 
-  // Convert the operands to scalable vectors.
-  SDValue Op0 = convertToScalableVector(DAG, ContainerVT, Op.getOperand(0));
-  SDValue Op1 = convertToScalableVector(DAG, ContainerVT, Op.getOperand(1));
+  // If wider type is not legal: split, extend, op, trunc and concat.
+  auto BisectExtendVector = [&DAG, &dl, &HalfVT, &PromVT,
----------------
peterwaller-arm wrote:
> sdesmalen wrote:
> > nit: This comment doesn't directly relate to BisecExtendVector
> If this comment were moved up one line and was followed by a blank, would that fix the nit?
Maybe better to move it to the block below where it uses the lambda, because that's where it will perform this sequence of operations.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:22631
+  // If wider type is not legal: split, extend, op, trunc and concat.
+  auto BisectExtendVector = [&DAG, &dl, &HalfVT, &PromVT,
+                             &ExtendOpcode](SDValue Op) {
----------------
peterwaller-arm wrote:
> sdesmalen wrote:
> > nit: ExtractLoHi ?
> I'm using "BisectExtend" as a compound verb to add more information about the intent of the operation (halving, extending). The knowledge it's extracting lo/hi is already available from the other operations occurring within the function.
Specifically the 'Bisect' part of the name confused me more than it helped me. Perhaps ExtractAndExtendLoHi is more clear?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139618