This patch updates the load insert point of the merged load in AggressiveInstCombine() as implemeted in
https://reviews.llvm.org/D135137
This is done to handle the reported test breaks.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp | ||
---|---|---|
755 | Is it possible for InsertPoint to be nullptr here? |
We found another case, that looks like this where the p3 is read out of order. We are looking into getting that fixed too.
define i32 @loadCombine_4consecutive_badinsert(ptr %p) { ; LE-LABEL: @loadCombine_4consecutive_badinsert( ; LE-NEXT: [[P3:%.*]] = getelementptr i8, ptr [[P:%.*]], i32 3 ; LE-NEXT: [[L1:%.*]] = load i32, ptr [[P]], align 1 ; LE-NEXT: store i8 0, ptr [[P3]], align 1 ; LE-NEXT: ret i32 [[L1]] ; %p1 = getelementptr i8, ptr %p, i32 1 %p2 = getelementptr i8, ptr %p, i32 2 %p3 = getelementptr i8, ptr %p, i32 3 %l2 = load i8, ptr %p1 store i8 0, ptr %p3, align 1 %l3 = load i8, ptr %p2 %l4 = load i8, ptr %p3 %l1 = load i8, ptr %p %e1 = zext i8 %l1 to i32 %e2 = zext i8 %l2 to i32 %e3 = zext i8 %l3 to i32 %e4 = zext i8 %l4 to i32 %s2 = shl i32 %e2, 8 %s3 = shl i32 %e3, 16 %s4 = shl i32 %e4, 24 %o1 = or i32 %e1, %s2 %o2 = or i32 %o1, %s3 %o3 = or i32 %o2, %s4 ret i32 %o3 }
Update the patch to handle various corner cases of Alias Analysis by handling the insert point of the load and associated pointer. For 2 loads move the insert point to the one which occurs first. Additionally look for clobber in the merged load when the merged load occurs later.
llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp | ||
---|---|---|
800 | If the pointer operand isn't in the same block, is that a problem? If they are in different blocks, and we know all the loads are in the same block, then we know the pointer operand dominates the RootInsert I think. It wont need the moveBefore below. |
llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp | ||
---|---|---|
800 | Good Point. Will update the patch with this change. |
Handle review comments on Load and Load pointer in separate BB's.
@dstuttard Would be good if the patch can be tested with non-Opaque Pointers as well. I have done a sanity test and it should work OK. Thanks.
Hi, this patch is creating malformed IR for some cases. Test case here: https://github.com/llvm/llvm-project/issues/62756
Thanks for reporting. I can see the issue and have fixed it in https://reviews.llvm.org/D150864.
I have not optimised it exclusively in the AggressiveInstCombine Pass because the InstCombine does the same and generates a single GEP for the nested patterns which can be reduced. This is then Load merged by the AggressiveInstCombine.
Is it possible for InsertPoint to be nullptr here?