[PATCH] D48332: [AArch64] Add custom lowering for v4i8 trunc store

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2689
+// Custom lowering for trunc store for vector types.
+SDValue AArch64TargetLowering::LowerSTORE(SDValue Op,
+                                          SelectionDAG &DAG) const {
----------------
`LowerSTORE` is too generic a name for this specific function, but I get it it's the pattern in the custom lowering.

You can keep the generic name as this will be the entry point for *all* custom store lowering, even if it only implements one type right now. But I'd add a longer comment explaining, for now, this only lowers truncating vector stores, but would be the place to add *any* custom store lowering (vector or not, truncating or not).



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2692
+  SDLoc Dl(Op);
+  StoreSDNode *StoreNode = cast<StoreSDNode>(Op);
+  unsigned AS = StoreNode->getAddressSpace();
----------------
asserts on `StoreNode` being not null


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2693
+  StoreSDNode *StoreNode = cast<StoreSDNode>(Op);
+  unsigned AS = StoreNode->getAddressSpace();
+
----------------
declare `AS` close to its usage to make it clear it's not left over dead code.


================
Comment at: test/CodeGen/AArch64/neon-truncStore-extLoad.ll:27
+; CHECK: xtn [[TMP2:(v[0-9]+)]].8b, [[TMP]].8h
+; CHECK: {{st1 { [[TMP2]].4h }[0]|str s[0-9]+}}, [x{{[0-9]+|sp}}]
+  %b = trunc <4 x i32> %a to <4 x i8>
----------------
efriedma wrote:
> Why does this CHECK line have two possible lowerings?
looks like it's a copy of the pattern around... weird...

What's the instruction actually generated?


Repository:
  rL LLVM

https://reviews.llvm.org/D48332