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

[Irina Dobrescu via Phabricator via llvm-commits]

Rin 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) {
----------------
paquette wrote:
> 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?
Hmm, that's a good point. I need to look into that.


================
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,
----------------
paquette wrote:
> 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.
Yeah, sure thing, I'll rename it


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:2148
   }
+
+  case TargetOpcode::G_ZEXT:
----------------
paquette wrote:
> Why did this code move?
For the i64 case for G_SEXT we saw that it was choosing the tablegen patterns instead of the switch(Opcode). To avoid that, this code was moved to earlySelect.


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/arm64-atomic.ll:324
 ; CHECK-NOLSE-O1-NEXT:    mov w0, w8
 ; CHECK-NOLSE-O1-NEXT:    ret
 ;
----------------
paquette wrote:
> I don't think this test change is necessary.
I think the test case was failing without the charge, but I'll switch it back and see if it works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108137