[PATCH] D108137: [AArch64][Global ISel] Add sext/zext improvements

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h:296
+template <typename LHS, typename RHS>
+inline BinaryOp_match<LHS, RHS, TargetOpcode::G_EXTRACT_VECTOR_ELT, true>
+m_GVExt(const LHS &L, const RHS &R) {
----------------
The boolean parameter on the end is true if this is commutative. I don't think G_EXTRACT_VECTOR_ELT is? Should this be false?


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h:297
+inline BinaryOp_match<LHS, RHS, TargetOpcode::G_EXTRACT_VECTOR_ELT, true>
+m_GVExt(const LHS &L, const RHS &R) {
+  return BinaryOp_match<LHS, RHS, TargetOpcode::G_EXTRACT_VECTOR_ELT, true>(L,
----------------
Can we use a name that makes this clear that this is matching G_EXTRACT_VECTOR_ELT? I can imagine people reading code using this thinking that it's matching some strange type of vector extend.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2148
   }
+
+  case TargetOpcode::G_ZEXT:
----------------
Why did this code move?


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/arm64-atomic.ll:324
 ; CHECK-NOLSE-O1-NEXT:    mov w0, w8
 ; CHECK-NOLSE-O1-NEXT:    ret
 ;
----------------
I don't think this test change is necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108137