[PATCH] D105978: [SVE][IR] Fix Binary op matching in PatternMatch::m_VScale

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

I'm surprised how we've never hit this until now. Mostly looks good to me, just a few nits.



================
Comment at: llvm/include/llvm/IR/PatternMatch.h:2435
+        auto *DerefTy = GEP->getSourceElementType();
+        if (GEP->getNumOperands() == 2 &&
+            m_Zero().match(GEP->getPointerOperand()) &&
----------------
nit: can you make this: `GEP->getNumIndices() == 1`?


================
Comment at: llvm/include/llvm/IR/PatternMatch.h:2438
+            m_SpecificInt(1).match(GEP->idx_begin()->get()) &&
+            isa<ScalableVectorType>(DerefTy) &&
+            DL.getTypeAllocSizeInBits(DerefTy).getKnownMinSize() == 8)
----------------
nit: please move this condition earlier, so that a simple condition that's likely to evaluate to `false`, is evaluated first.


================
Comment at: llvm/unittests/IR/PatternMatch.cpp:1640
+TEST_F(PatternMatchTest, VScale) {
+
+  DataLayout DL = M->getDataLayout();
----------------
nit: unnecessary newline


================
Comment at: llvm/unittests/IR/PatternMatch.cpp:1645
+  Type *VecPtrTy = PointerType::get(VecTy, 0);
+  Value *NullPtrVec = Constant::getNullValue(VecPtrTy);
+  Value *GEP = IRB.CreateGEP(VecTy, NullPtrVec, IRB.getInt64(1));
----------------
You can write `VecTy->getPointerTo()` and remove `VecPtrTy`.


================
Comment at: llvm/unittests/IR/PatternMatch.cpp:1651
+  Type *VecTy2 = ScalableVectorType::get(IRB.getInt8Ty(), 2);
+  Type *VecPtrTy2 = PointerType::get(VecTy2, 0);
+  Value *NullPtrVec2 = Constant::getNullValue(VecPtrTy2);
----------------
Same comment as above, you can write `VecTy2->getPointerTo()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105978