[PATCH] D54265: [DAGCombiner] Fix load-store forwarding of indexed loads.
Peter Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 9 03:12:46 PST 2018
peter.smith added a comment.
This is looking good. I've got one style nit that I'll leave to your discretion. I think it is probably worth tidying up the test case a little more though. I've made a proposal in the comment.
================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12865
+
+ auto ReplaceLd = [&](LoadSDNode *LD, SDValue Val, SDValue Chain) -> SDValue {
+ if (LD->isIndexed()) {
----------------
Taking up one of the comments from the original review for D49200. The use of auto could be considered a bit excessive here. https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
For example use bool for IsSub, ISD::NodeType for Opc and SDValue for Idx. I don't have a strong opinion about this personally but it might be more consistent with the other code in the file.
================
Comment at: llvm/test/CodeGen/ARM/pr39571.ll:1
+; RUN: llc < %s -mtriple armv4t-unknown-linux-gnueabi -mattr=+strict-align
+
----------------
I've made an attempt to strip down the test case a bit. I've also upped the target Arm architecture to a more recent one. It seems like the original test passed with armv7a due to the availability of unaligned access, but it will fail if -mno-unaligned-access is used.
I've changed the variable names to remove the association with the original program.
```
; RUN: llc < %s -mtriple armv7a-unknown-linux-gnueabi -mattr=+strict-align
; Avoid crash from forwarding indexed-loads back to store.
%struct.anon = type { %struct.ma*, %struct.mb }
%struct.ma = type { i8 }
%struct.mb = type { i8, i8 }
%struct.anon.0 = type { %struct.anon.1 }
%struct.anon.1 = type { %struct.ds }
%struct.ds = type <{ i8, %union.ie }>
%union.ie = type { %struct.ib }
%struct.ib = type { i8, i8, i16 }
@a = common dso_local local_unnamed_addr global %struct.anon* null, align 4
@b = common dso_local local_unnamed_addr global %struct.anon.0 zeroinitializer, align 1
; Function Attrs: norecurse nounwind
define dso_local void @func() local_unnamed_addr {
entry:
%0 = load %struct.anon*, %struct.anon** @a, align 4
%ad = getelementptr inbounds %struct.anon, %struct.anon* %0, i32 0, i32 0
%1 = load %struct.ma*, %struct.ma** %ad, align 4
%c.sroa.0.0..sroa_idx = getelementptr inbounds %struct.ma, %struct.ma* %1, i32 0, i32 0
%c.sroa.0.0.copyload = load i8, i8* %c.sroa.0.0..sroa_idx, align 1
%cb = getelementptr inbounds %struct.anon, %struct.anon* %0, i32 0, i32 1
%band = getelementptr inbounds %struct.anon, %struct.anon* %0, i32 0, i32 1, i32 1
store i8 %c.sroa.0.0.copyload, i8* %band, align 4
store i8 6, i8* getelementptr inbounds (%struct.anon.0, %struct.anon.0* @b, i32 0, i32 0, i32 0, i32 1, i32 0, i32 0), align 1
store i8 2, i8* getelementptr inbounds (%struct.anon.0, %struct.anon.0* @b, i32 0, i32 0, i32 0, i32 1, i32 0, i32 1), align 1
%2 = bitcast %struct.mb* %cb to i32*
%3 = load i32, i32* bitcast (i8* getelementptr inbounds (%struct.anon.0, %struct.anon.0* @b, i32 0, i32 0, i32 0, i32 1, i32 0, i32 0) to i32*), align 1
store i32 %3, i32* %2, align 1
ret void
}
```
Repository:
rL LLVM
https://reviews.llvm.org/D54265
More information about the llvm-commits
mailing list