[EarlyCSE] Rematerialize alignment assumption. #109131
Conversation
fhahn
force-pushed
the
earlycse-align-assumption
branch
from
415832b
to
a9eb56c
Compare
fhahn
force-pushed
the
earlycse-align-assumption
branch
from
a9eb56c
to
b4f54c4
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
fhahn
force-pushed
the
earlycse-align-assumption
branch
from
b4f54c4
to
903d6d9
Compare
|
@llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesIf the load has align metadata, preserve it via an alignment Comparison run dtcxzyw/llvm-opt-benchmark#1343 (comment) Unfortunately the diff is too large to display, top changes Full diff: https://github.com/llvm/llvm-project/pull/109131.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
index cf11f5bc885a75..743b333987bc9a 100644
--- a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
+++ b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
@@ -31,6 +31,7 @@
#include "llvm/IR/Constants.h"
#include "llvm/IR/Dominators.h"
#include "llvm/IR/Function.h"
+#include "llvm/IR/IRBuilder.h"
#include "llvm/IR/InstrTypes.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Instructions.h"
@@ -1588,6 +1589,18 @@ bool EarlyCSE::processNode(DomTreeNode *Node) {
if (InVal.IsLoad)
if (auto *I = dyn_cast<Instruction>(Op))
combineMetadataForCSE(I, &Inst, false);
+
+ // If the load has align metadata, preserve it via an alignment
+ // assumption. Note that this doesn't use salavageKnowledge, as we need
+ // to create the assumption for the value we replaced the load with.
+ if (auto *AlignMD = Inst.getMetadata(LLVMContext::MD_align)) {
+ auto *A = mdconst::extract<ConstantInt>(AlignMD->getOperand(0));
+ if (Op->getPointerAlignment(SQ.DL).value() % A->getZExtValue() != 0) {
+ IRBuilder B(&Inst);
+ B.CreateAlignmentAssumption(SQ.DL, Op, A);
+ }
+ }
+
if (!Inst.use_empty())
Inst.replaceAllUsesWith(Op);
salvageKnowledge(&Inst, &AC);
diff --git a/llvm/test/Transforms/EarlyCSE/materialize-align-assumptions.ll b/llvm/test/Transforms/EarlyCSE/materialize-align-assumptions.ll
index ea63376957162b..628577b0975071 100644
--- a/llvm/test/Transforms/EarlyCSE/materialize-align-assumptions.ll
+++ b/llvm/test/Transforms/EarlyCSE/materialize-align-assumptions.ll
@@ -10,6 +10,7 @@ define ptr @align_replacement_does_not_have_align_metadata(ptr noalias %p) {
; CHECK-NEXT: call void @foo(ptr [[L_1]])
; CHECK-NEXT: [[GEP:%.*]] = getelementptr i8, ptr [[L_1]], i64 4
; CHECK-NEXT: store ptr [[GEP]], ptr [[P]], align 8
+; CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(ptr [[GEP]], i64 4) ]
; CHECK-NEXT: ret ptr [[GEP]]
;
%l.1 = load ptr, ptr %p, align 8
@@ -27,6 +28,7 @@ define ptr @align_replacement_does_not_have_align_metadata2(ptr noalias %p) {
; CHECK-NEXT: [[L_1:%.*]] = load ptr, ptr [[P]], align 8
; CHECK-NEXT: [[GEP:%.*]] = getelementptr i8, ptr [[L_1]], i64 4
; CHECK-NEXT: store ptr [[GEP]], ptr [[P]], align 8
+; CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(ptr [[GEP]], i64 4) ]
; CHECK-NEXT: ret ptr [[GEP]]
;
%l.1 = load ptr, ptr %p, align 8
@@ -54,6 +56,7 @@ define ptr @align_replacement_has_smaller_alignment(ptr noalias %p) {
; CHECK-SAME: ptr noalias [[P:%.*]]) {
; CHECK-NEXT: [[L_1:%.*]] = load ptr, ptr [[P]], align 8, !align [[META0]]
; CHECK-NEXT: call void @foo(ptr [[L_1]])
+; CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(ptr [[L_1]], i64 8) ]
; CHECK-NEXT: ret ptr [[L_1]]
;
%l.1 = load ptr, ptr %p, align 8, !align !0
|
| // If the load has align metadata, preserve it via an alignment | ||
| // assumption. Note that this doesn't use salavageKnowledge, as we need | ||
| // to create the assumption for the value we replaced the load with. | ||
| if (auto *AlignMD = Inst.getMetadata(LLVMContext::MD_align)) { |
There was a problem hiding this comment.
You also need !noundef for this transform. !align by itself only returns poison, while the assumption converts it into IUB.
There was a problem hiding this comment.
Should be updated, thanks!
fhahn
force-pushed
the
earlycse-align-assumption
branch
from
903d6d9
to
31a095c
Compare
Precommit tests for llvm#109131.
Fixed. See dtcxzyw/llvm-opt-benchmark#1345. |
|
I'm still not convinced that doing something like this is a good idea. The core problem is that you can't tell whether the I looked through the IR diffs, and there were plenty of minor optimization regressions and very few improvements. I think the two most common ones are:
I also noticed that in some cases the same assume is repeated many times in the same block -- we should be optimizing those away. Another interesting data point is that nearly all assumptions were redundant in the sense that they were directly followed by a load or gep+load with the same alignment. (Of course, most of our analysis cannot really use alignment on loads to reason about the alignment of the pointer at some other point.) |
Yeah, when originally working on #108958, I didn't expect much fallout as !align/alignment assumptions isn't super common in C/C++, forgetting about Rust.
Yep, unfortunately there's a number of improvements that are still needed to avoid negative fallout from using assumptions...
We might be able to skip rematerializing in those cases by doing some extra analysis. I can look into addressing some of the issues mentioned above. Overall my main goal is to remove the blockers for #108958 :) |
If the load has align metadata, preserve it via an alignment
assumption. Note that this doesn't use salavageKnowledge, as we need
to create the assumption for the value we replaced the load with.
Comparison run dtcxzyw/llvm-opt-benchmark#1343 (comment)
Unfortunately the diff is too large to display, top changes