Skip to content

Commit bc23709

Browse files
committed
[Local] Make combineAAMetadata() more principled
This moves combineAAMetadata() into Local and implements it via a new AAOnly flag, which will intersect only AA metadata and keep other known metadata. The existing KnownIDs list is dropped, because it is redundant with the switch in combineMetadata(), which already drops unknown metadata. I checked that the memcpy tests still pass if we adjust the logic for MD_memprof/MD_callsite to drop the metadata instead of arbitrarily picking one. Fixes #121495.
1 parent bfa711a commit bc23709

File tree

3 files changed

+31
-71
lines changed

3 files changed

+31
-71
lines changed

llvm/include/llvm/Transforms/Utils/Local.h

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -412,19 +412,6 @@ Instruction *removeUnwindEdge(BasicBlock *BB, DomTreeUpdater *DTU = nullptr);
412412
bool removeUnreachableBlocks(Function &F, DomTreeUpdater *DTU = nullptr,
413413
MemorySSAUpdater *MSSAU = nullptr);
414414

415-
/// DO NOT CALL EXTERNALLY.
416-
/// FIXME: https://github.com/llvm/llvm-project/issues/121495
417-
/// Once external callers of this function are removed, either inline into
418-
/// combineMetadataForCSE, or internalize and remove KnownIDs parameter.
419-
///
420-
/// Combine the metadata of two instructions so that K can replace J. Some
421-
/// metadata kinds can only be kept if K does not move, meaning it dominated
422-
/// J in the original IR.
423-
///
424-
/// Metadata not listed as known via KnownIDs is removed
425-
void combineMetadata(Instruction *K, const Instruction *J,
426-
ArrayRef<unsigned> KnownIDs, bool DoesKMove);
427-
428415
/// Combine the metadata of two instructions so that K can replace J. This
429416
/// specifically handles the case of CSE-like transformations. Some
430417
/// metadata can only be kept if K dominates J. For this to be correct,
@@ -434,6 +421,11 @@ void combineMetadata(Instruction *K, const Instruction *J,
434421
void combineMetadataForCSE(Instruction *K, const Instruction *J,
435422
bool DoesKMove);
436423

424+
/// Combine metadata of two instructions, where instruction J is a memory
425+
/// access that has been merged into K. This will intersect alias-analysis
426+
/// metadata, while preserving other known metadata.
427+
void combineAAMetadata(Instruction *K, const Instruction *J);
428+
437429
/// Copy the metadata from the source instruction to the destination (the
438430
/// replacement for the source instruction).
439431
void copyMetadataForLoad(LoadInst &Dest, const LoadInst &Source);

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -341,21 +341,6 @@ static bool writtenBetween(MemorySSA *MSSA, BatchAAResults &AA,
341341
return !MSSA->dominates(Clobber, Start);
342342
}
343343

344-
// Update AA metadata
345-
static void combineAAMetadata(Instruction *ReplInst, Instruction *I) {
346-
// FIXME: MD_tbaa_struct and MD_mem_parallel_loop_access should also be
347-
// handled here, but combineMetadata doesn't support them yet
348-
unsigned KnownIDs[] = {
349-
LLVMContext::MD_tbaa, LLVMContext::MD_alias_scope,
350-
LLVMContext::MD_noalias, LLVMContext::MD_invariant_group,
351-
LLVMContext::MD_access_group, LLVMContext::MD_prof,
352-
LLVMContext::MD_memprof, LLVMContext::MD_callsite};
353-
// FIXME: https://github.com/llvm/llvm-project/issues/121495
354-
// Use custom AA metadata combining handling instead of combineMetadata, which
355-
// is meant for CSE and will drop any metadata not in the KnownIDs list.
356-
combineMetadata(ReplInst, I, KnownIDs, true);
357-
}
358-
359344
/// When scanning forward over instructions, we look for some other patterns to
360345
/// fold away. In particular, this looks for stores to neighboring locations of
361346
/// memory. If it sees enough consecutive ones, it attempts to merge them

llvm/lib/Transforms/Utils/Local.cpp

Lines changed: 26 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -3308,13 +3308,11 @@ bool llvm::removeUnreachableBlocks(Function &F, DomTreeUpdater *DTU,
33083308
return Changed;
33093309
}
33103310

3311-
// FIXME: https://github.com/llvm/llvm-project/issues/121495
3312-
// Once external callers of this function are removed, either inline into
3313-
// combineMetadataForCSE, or internalize and remove KnownIDs parameter.
3314-
void llvm::combineMetadata(Instruction *K, const Instruction *J,
3315-
ArrayRef<unsigned> KnownIDs, bool DoesKMove) {
3311+
/// If AAOnly is set, only intersect alias analysis metadata and preserve other
3312+
/// known metadata. Unknown metadata is always dropped.
3313+
static void combineMetadata(Instruction *K, const Instruction *J,
3314+
bool DoesKMove, bool AAOnly = false) {
33163315
SmallVector<std::pair<unsigned, MDNode *>, 4> Metadata;
3317-
K->dropUnknownNonDebugMetadata(KnownIDs);
33183316
K->getAllMetadataOtherThanDebugLoc(Metadata);
33193317
for (const auto &MD : Metadata) {
33203318
unsigned Kind = MD.first;
@@ -3323,16 +3321,13 @@ void llvm::combineMetadata(Instruction *K, const Instruction *J,
33233321

33243322
switch (Kind) {
33253323
default:
3326-
// FIXME: https://github.com/llvm/llvm-project/issues/121495
3327-
// Change to removing only explicitly listed other metadata, and assert
3328-
// on unknown metadata, to avoid inadvertently dropping newly added
3329-
// metadata types.
33303324
K->setMetadata(Kind, nullptr); // Remove unknown metadata
33313325
break;
33323326
case LLVMContext::MD_dbg:
33333327
llvm_unreachable("getAllMetadataOtherThanDebugLoc returned a MD_dbg");
33343328
case LLVMContext::MD_DIAssignID:
3335-
K->mergeDIAssignID(J);
3329+
if (!AAOnly)
3330+
K->mergeDIAssignID(J);
33363331
break;
33373332
case LLVMContext::MD_tbaa:
33383333
if (DoesKMove)
@@ -3353,11 +3348,12 @@ void llvm::combineMetadata(Instruction *K, const Instruction *J,
33533348
intersectAccessGroups(K, J));
33543349
break;
33553350
case LLVMContext::MD_range:
3356-
if (DoesKMove || !K->hasMetadata(LLVMContext::MD_noundef))
3351+
if (!AAOnly && (DoesKMove || !K->hasMetadata(LLVMContext::MD_noundef)))
33573352
K->setMetadata(Kind, MDNode::getMostGenericRange(JMD, KMD));
33583353
break;
33593354
case LLVMContext::MD_fpmath:
3360-
K->setMetadata(Kind, MDNode::getMostGenericFPMath(JMD, KMD));
3355+
if (!AAOnly)
3356+
K->setMetadata(Kind, MDNode::getMostGenericFPMath(JMD, KMD));
33613357
break;
33623358
case LLVMContext::MD_invariant_load:
33633359
// If K moves, only set the !invariant.load if it is present in both
@@ -3366,7 +3362,7 @@ void llvm::combineMetadata(Instruction *K, const Instruction *J,
33663362
K->setMetadata(Kind, JMD);
33673363
break;
33683364
case LLVMContext::MD_nonnull:
3369-
if (DoesKMove || !K->hasMetadata(LLVMContext::MD_noundef))
3365+
if (!AAOnly && (DoesKMove || !K->hasMetadata(LLVMContext::MD_noundef)))
33703366
K->setMetadata(Kind, JMD);
33713367
break;
33723368
case LLVMContext::MD_invariant_group:
@@ -3376,36 +3372,39 @@ void llvm::combineMetadata(Instruction *K, const Instruction *J,
33763372
// Combine MMRAs
33773373
break;
33783374
case LLVMContext::MD_align:
3379-
if (DoesKMove || !K->hasMetadata(LLVMContext::MD_noundef))
3375+
if (!AAOnly && (DoesKMove || !K->hasMetadata(LLVMContext::MD_noundef)))
33803376
K->setMetadata(
33813377
Kind, MDNode::getMostGenericAlignmentOrDereferenceable(JMD, KMD));
33823378
break;
33833379
case LLVMContext::MD_dereferenceable:
33843380
case LLVMContext::MD_dereferenceable_or_null:
3385-
if (DoesKMove)
3381+
if (!AAOnly && DoesKMove)
33863382
K->setMetadata(Kind,
33873383
MDNode::getMostGenericAlignmentOrDereferenceable(JMD, KMD));
33883384
break;
33893385
case LLVMContext::MD_memprof:
3390-
K->setMetadata(Kind, MDNode::getMergedMemProfMetadata(KMD, JMD));
3386+
if (!AAOnly)
3387+
K->setMetadata(Kind, MDNode::getMergedMemProfMetadata(KMD, JMD));
33913388
break;
33923389
case LLVMContext::MD_callsite:
3393-
K->setMetadata(Kind, MDNode::getMergedCallsiteMetadata(KMD, JMD));
3390+
if (!AAOnly)
3391+
K->setMetadata(Kind, MDNode::getMergedCallsiteMetadata(KMD, JMD));
33943392
break;
33953393
case LLVMContext::MD_preserve_access_index:
33963394
// Preserve !preserve.access.index in K.
33973395
break;
33983396
case LLVMContext::MD_noundef:
33993397
// If K does move, keep noundef if it is present in both instructions.
3400-
if (DoesKMove)
3398+
if (!AAOnly && DoesKMove)
34013399
K->setMetadata(Kind, JMD);
34023400
break;
34033401
case LLVMContext::MD_nontemporal:
34043402
// Preserve !nontemporal if it is present on both instructions.
3405-
K->setMetadata(Kind, JMD);
3403+
if (!AAOnly)
3404+
K->setMetadata(Kind, JMD);
34063405
break;
34073406
case LLVMContext::MD_prof:
3408-
if (DoesKMove)
3407+
if (!AAOnly && DoesKMove)
34093408
K->setMetadata(Kind, MDNode::getMergedProfMetadata(KMD, JMD, K, J));
34103409
break;
34113410
case LLVMContext::MD_noalias_addrspace:
@@ -3437,28 +3436,12 @@ void llvm::combineMetadata(Instruction *K, const Instruction *J,
34373436
}
34383437

34393438
void llvm::combineMetadataForCSE(Instruction *K, const Instruction *J,
3440-
bool KDominatesJ) {
3441-
unsigned KnownIDs[] = {LLVMContext::MD_tbaa,
3442-
LLVMContext::MD_alias_scope,
3443-
LLVMContext::MD_noalias,
3444-
LLVMContext::MD_range,
3445-
LLVMContext::MD_fpmath,
3446-
LLVMContext::MD_invariant_load,
3447-
LLVMContext::MD_nonnull,
3448-
LLVMContext::MD_invariant_group,
3449-
LLVMContext::MD_align,
3450-
LLVMContext::MD_dereferenceable,
3451-
LLVMContext::MD_dereferenceable_or_null,
3452-
LLVMContext::MD_access_group,
3453-
LLVMContext::MD_preserve_access_index,
3454-
LLVMContext::MD_prof,
3455-
LLVMContext::MD_nontemporal,
3456-
LLVMContext::MD_noundef,
3457-
LLVMContext::MD_mmra,
3458-
LLVMContext::MD_noalias_addrspace,
3459-
LLVMContext::MD_memprof,
3460-
LLVMContext::MD_callsite};
3461-
combineMetadata(K, J, KnownIDs, KDominatesJ);
3439+
bool DoesKMove) {
3440+
combineMetadata(K, J, DoesKMove);
3441+
}
3442+
3443+
void llvm::combineAAMetadata(Instruction *K, const Instruction *J) {
3444+
combineMetadata(K, J, /*DoesKMove=*/true, /*AAOnly=*/true);
34623445
}
34633446

34643447
void llvm::copyMetadataForLoad(LoadInst &Dest, const LoadInst &Source) {

0 commit comments

Comments
 (0)