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

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz 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 {
----------------
rengolin wrote:
> `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).
> 
Ack, I will add a comment.


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


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


================
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>
----------------
rengolin wrote:
> 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?
Indeed I used the previous function as example (truncStore.v4i32), and for current testing the instruction being generated is 'str'.


Repository:
  rL LLVM

https://reviews.llvm.org/D48332