Skip to content

Commit accc6b5

Browse files
efriedma-quicefriedma-quic
committed
LoadInst should store Align, not MaybeAlign.
The fact that loads and stores can have the alignment missing is a constant source of confusion: code that usually works can break down in rare cases. So fix the LoadInst API so the alignment is never missing. To reduce the number of changes required to make this work, IRBuilder and certain LoadInst constructors will grab the module's datalayout and compute the alignment automatically. This is the same alignment instcombine would eventually apply anyway; we're just doing it earlier. There's a minor risk that the way we're retrieving the datalayout could break out-of-tree code, but I don't think that's likely. This is the last in a series of patches, so most of the necessary changes have already been merged. Differential Revision: https://reviews.llvm.org/D77454
1 parent 2b7fe08 commit accc6b5

File tree

10 files changed

+52
-48
lines changed

10 files changed

+52
-48
lines changed

‎llvm/include/llvm/IR/Instructions.h

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -180,23 +180,23 @@ class LoadInst : public UnaryInstruction {
180180
LoadInst *cloneImpl() const;
181181

182182
public:
183-
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr = "",
184-
Instruction *InsertBefore = nullptr);
183+
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr,
184+
Instruction *InsertBefore);
185185
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, BasicBlock *InsertAtEnd);
186186
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
187-
Instruction *InsertBefore = nullptr);
187+
Instruction *InsertBefore);
188188
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
189189
BasicBlock *InsertAtEnd);
190190
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
191-
MaybeAlign Align, Instruction *InsertBefore = nullptr);
191+
Align Align, Instruction *InsertBefore = nullptr);
192192
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
193-
MaybeAlign Align, BasicBlock *InsertAtEnd);
193+
Align Align, BasicBlock *InsertAtEnd);
194194
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
195-
MaybeAlign Align, AtomicOrdering Order,
195+
Align Align, AtomicOrdering Order,
196196
SyncScope::ID SSID = SyncScope::System,
197197
Instruction *InsertBefore = nullptr);
198198
LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, bool isVolatile,
199-
MaybeAlign Align, AtomicOrdering Order, SyncScope::ID SSID,
199+
Align Align, AtomicOrdering Order, SyncScope::ID SSID,
200200
BasicBlock *InsertAtEnd);
201201

202202
/// Return true if this is a load from a volatile memory location.
@@ -211,18 +211,14 @@ class LoadInst : public UnaryInstruction {
211211
/// Return the alignment of the access that is being performed.
212212
/// FIXME: Remove this function once transition to Align is over.
213213
/// Use getAlign() instead.
214-
unsigned getAlignment() const {
215-
if (const auto MA = getAlign())
216-
return MA->value();
217-
return 0;
218-
}
214+
unsigned getAlignment() const { return getAlign().value(); }
219215

220216
/// Return the alignment of the access that is being performed.
221-
MaybeAlign getAlign() const {
222-
return decodeMaybeAlign((getSubclassDataFromInstruction() >> 1) & 31);
217+
Align getAlign() const {
218+
return *decodeMaybeAlign((getSubclassDataFromInstruction() >> 1) & 31);
223219
}
224220

225-
void setAlignment(MaybeAlign Alignment);
221+
void setAlignment(Align Alignment);
226222

227223
/// Returns the ordering constraint of this load instruction.
228224
AtomicOrdering getOrdering() const {

‎llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4563,7 +4563,7 @@ void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
45634563

45644564
MachineMemOperand *MMO = DAG.getMachineFunction().getMachineMemOperand(
45654565
MachinePointerInfo(I.getPointerOperand()), Flags, MemVT.getStoreSize(),
4566-
*I.getAlign(), AAMDNodes(), nullptr, SSID, Order);
4566+
I.getAlign(), AAMDNodes(), nullptr, SSID, Order);
45674567

45684568
InChain = TLI.prepareVolatileOrAtomicLoad(InChain, dl, DAG);
45694569

‎llvm/lib/IR/Core.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2015,7 +2015,7 @@ void LLVMSetAlignment(LLVMValueRef V, unsigned Bytes) {
20152015
else if (AllocaInst *AI = dyn_cast<AllocaInst>(P))
20162016
AI->setAlignment(MaybeAlign(Bytes));
20172017
else if (LoadInst *LI = dyn_cast<LoadInst>(P))
2018-
LI->setAlignment(MaybeAlign(Bytes));
2018+
LI->setAlignment(Align(Bytes));
20192019
else if (StoreInst *SI = dyn_cast<StoreInst>(P))
20202020
SI->setAlignment(MaybeAlign(Bytes));
20212021
else

‎llvm/lib/IR/Instructions.cpp

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,6 +1326,15 @@ void LoadInst::AssertOK() {
13261326
"Alignment required for atomic load");
13271327
}
13281328

1329+
Align computeLoadAlign(Type *Ty, BasicBlock *BB) {
1330+
const DataLayout &DL = BB->getModule()->getDataLayout();
1331+
return DL.getABITypeAlign(Ty);
1332+
}
1333+
1334+
Align computeLoadAlign(Type *Ty, Instruction *I) {
1335+
return computeLoadAlign(Ty, I->getParent());
1336+
}
1337+
13291338
LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name,
13301339
Instruction *InsertBef)
13311340
: LoadInst(Ty, Ptr, Name, /*isVolatile=*/false, InsertBef) {}
@@ -1336,36 +1345,38 @@ LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name,
13361345

13371346
LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
13381347
Instruction *InsertBef)
1339-
: LoadInst(Ty, Ptr, Name, isVolatile, /*Align=*/None, InsertBef) {}
1348+
: LoadInst(Ty, Ptr, Name, isVolatile, computeLoadAlign(Ty, InsertBef),
1349+
InsertBef) {}
13401350

13411351
LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
13421352
BasicBlock *InsertAE)
1343-
: LoadInst(Ty, Ptr, Name, isVolatile, /*Align=*/None, InsertAE) {}
1353+
: LoadInst(Ty, Ptr, Name, isVolatile, computeLoadAlign(Ty, InsertAE),
1354+
InsertAE) {}
13441355

13451356
LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
1346-
MaybeAlign Align, Instruction *InsertBef)
1357+
Align Align, Instruction *InsertBef)
13471358
: LoadInst(Ty, Ptr, Name, isVolatile, Align, AtomicOrdering::NotAtomic,
13481359
SyncScope::System, InsertBef) {}
13491360

13501361
LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
1351-
MaybeAlign Align, BasicBlock *InsertAE)
1362+
Align Align, BasicBlock *InsertAE)
13521363
: LoadInst(Ty, Ptr, Name, isVolatile, Align, AtomicOrdering::NotAtomic,
13531364
SyncScope::System, InsertAE) {}
13541365

13551366
LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
1356-
MaybeAlign Align, AtomicOrdering Order, SyncScope::ID SSID,
1367+
Align Align, AtomicOrdering Order, SyncScope::ID SSID,
13571368
Instruction *InsertBef)
13581369
: UnaryInstruction(Ty, Load, Ptr, InsertBef) {
13591370
assert(Ty == cast<PointerType>(Ptr->getType())->getElementType());
13601371
setVolatile(isVolatile);
1361-
setAlignment(MaybeAlign(Align));
1372+
setAlignment(Align);
13621373
setAtomic(Order, SSID);
13631374
AssertOK();
13641375
setName(Name);
13651376
}
13661377

13671378
LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
1368-
MaybeAlign Align, AtomicOrdering Order, SyncScope::ID SSID,
1379+
Align Align, AtomicOrdering Order, SyncScope::ID SSID,
13691380
BasicBlock *InsertAE)
13701381
: UnaryInstruction(Ty, Load, Ptr, InsertAE) {
13711382
assert(Ty == cast<PointerType>(Ptr->getType())->getElementType());
@@ -1376,8 +1387,8 @@ LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
13761387
setName(Name);
13771388
}
13781389

1379-
void LoadInst::setAlignment(MaybeAlign Align) {
1380-
assert((!Align || *Align <= MaximumAlignment) &&
1390+
void LoadInst::setAlignment(Align Align) {
1391+
assert(Align <= MaximumAlignment &&
13811392
"Alignment is greater than MaximumAlignment!");
13821393
setInstructionSubclassData((getSubclassDataFromInstruction() & ~(31 << 1)) |
13831394
(encode(Align) << 1));
@@ -4233,8 +4244,7 @@ AllocaInst *AllocaInst::cloneImpl() const {
42334244

42344245
LoadInst *LoadInst::cloneImpl() const {
42354246
return new LoadInst(getType(), getOperand(0), Twine(), isVolatile(),
4236-
MaybeAlign(getAlignment()), getOrdering(),
4237-
getSyncScopeID());
4247+
getAlign(), getOrdering(), getSyncScopeID());
42384248
}
42394249

42404250
StoreInst *StoreInst::cloneImpl() const {

‎llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ bool NVPTXLowerAggrCopies::runOnFunction(Function &F) {
113113
createMemCpyLoopKnownSize(/* ConvertedInst */ SI,
114114
/* SrcAddr */ SrcAddr, /* DstAddr */ DstAddr,
115115
/* CopyLen */ CopyLen,
116-
/* SrcAlign */ LI->getAlign().valueOrOne(),
116+
/* SrcAlign */ LI->getAlign(),
117117
/* DestAlign */ SI->getAlign().valueOrOne(),
118118
/* SrcIsVolatile */ LI->isVolatile(),
119119
/* DstIsVolatile */ SI->isVolatile(), TTI);

‎llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,7 @@ Instruction *InstCombiner::SimplifyAnyMemTransfer(AnyMemTransferInst *MI) {
194194
Value *Dest = Builder.CreateBitCast(MI->getArgOperand(0), NewDstPtrTy);
195195
LoadInst *L = Builder.CreateLoad(IntType, Src);
196196
// Alignment from the mem intrinsic will be better, so use it.
197-
L->setAlignment(
198-
MaybeAlign(CopySrcAlign)); // FIXME: Check if we can use Align instead.
197+
L->setAlignment(*CopySrcAlign);
199198
if (CopyMD)
200199
L->setMetadata(LLVMContext::MD_tbaa, CopyMD);
201200
MDNode *LoopMemParallelMD =
@@ -2465,7 +2464,7 @@ Instruction *InstCombiner::visitCallInst(CallInst &CI) {
24652464
&DT) >= 16) {
24662465
Value *Ptr = Builder.CreateBitCast(II->getArgOperand(0),
24672466
PointerType::getUnqual(II->getType()));
2468-
return new LoadInst(II->getType(), Ptr);
2467+
return new LoadInst(II->getType(), Ptr, "", false, Align(16));
24692468
}
24702469
break;
24712470
case Intrinsic::ppc_vsx_lxvw4x:
@@ -2512,7 +2511,7 @@ Instruction *InstCombiner::visitCallInst(CallInst &CI) {
25122511
&DT) >= 32) {
25132512
Value *Ptr = Builder.CreateBitCast(II->getArgOperand(0),
25142513
PointerType::getUnqual(II->getType()));
2515-
return new LoadInst(II->getType(), Ptr);
2514+
return new LoadInst(II->getType(), Ptr, "", false, Align(32));
25162515
}
25172516
break;
25182517
case Intrinsic::ppc_qpx_qvstfs:

‎llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,8 @@ void PointerReplacer::replace(Instruction *I) {
281281
if (auto *LT = dyn_cast<LoadInst>(I)) {
282282
auto *V = getReplacement(LT->getPointerOperand());
283283
assert(V && "Operand not replaced");
284-
auto *NewI = new LoadInst(I->getType(), V);
284+
auto *NewI = new LoadInst(I->getType(), V, "", false,
285+
IC.getDataLayout().getABITypeAlign(I->getType()));
285286
NewI->takeName(LT);
286287
IC.InsertNewInstWith(NewI, *LT);
287288
IC.replaceInstUsesWith(*LT, NewI);
@@ -1008,7 +1009,7 @@ Instruction *InstCombiner::visitLoadInst(LoadInst &LI) {
10081009
//
10091010
if (SelectInst *SI = dyn_cast<SelectInst>(Op)) {
10101011
// load (select (Cond, &V1, &V2)) --> select(Cond, load &V1, load &V2).
1011-
const MaybeAlign Alignment(LI.getAlignment());
1012+
Align Alignment = LI.getAlign();
10121013
if (isSafeToLoadUnconditionally(SI->getOperand(1), LI.getType(),
10131014
Alignment, DL, SI) &&
10141015
isSafeToLoadUnconditionally(SI->getOperand(2), LI.getType(),

‎llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,7 @@ Instruction *InstCombiner::FoldPHIArgLoadIntoPHI(PHINode &PN) {
554554
// visitLoadInst will propagate an alignment onto the load when TD is around,
555555
// and if TD isn't around, we can't handle the mixed case.
556556
bool isVolatile = FirstLI->isVolatile();
557-
MaybeAlign LoadAlignment(FirstLI->getAlignment());
557+
Align LoadAlignment = FirstLI->getAlign();
558558
unsigned LoadAddrSpace = FirstLI->getPointerAddressSpace();
559559

560560
// We can't sink the load if the loaded value could be modified between the
@@ -584,12 +584,7 @@ Instruction *InstCombiner::FoldPHIArgLoadIntoPHI(PHINode &PN) {
584584
!isSafeAndProfitableToSinkLoad(LI))
585585
return nullptr;
586586

587-
// If some of the loads have an alignment specified but not all of them,
588-
// we can't do the transformation.
589-
if ((LoadAlignment.hasValue()) != (LI->getAlignment() != 0))
590-
return nullptr;
591-
592-
LoadAlignment = std::min(LoadAlignment, MaybeAlign(LI->getAlignment()));
587+
LoadAlignment = std::min(LoadAlignment, Align(LI->getAlign()));
593588

594589
// If the PHI is of volatile loads and the load block has multiple
595590
// successors, sinking it would remove a load of the volatile value from

‎llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,21 +320,24 @@ bool AlignmentFromAssumptionsPass::processAssumption(CallInst *ACall) {
320320
WorkList.push_back(K);
321321
}
322322

323+
const DataLayout &DL = SE->getDataLayout();
323324
while (!WorkList.empty()) {
324325
Instruction *J = WorkList.pop_back_val();
325326
if (LoadInst *LI = dyn_cast<LoadInst>(J)) {
326327
Align NewAlignment = getNewAlignment(AASCEV, AlignSCEV, OffSCEV,
327328
LI->getPointerOperand(), SE);
328-
329-
if (NewAlignment > *LI->getAlign()) {
329+
Align OldAlignment =
330+
DL.getValueOrABITypeAlignment(LI->getAlign(), LI->getType());
331+
if (NewAlignment > OldAlignment) {
330332
LI->setAlignment(NewAlignment);
331333
++NumLoadAlignChanged;
332334
}
333335
} else if (StoreInst *SI = dyn_cast<StoreInst>(J)) {
334336
Align NewAlignment = getNewAlignment(AASCEV, AlignSCEV, OffSCEV,
335337
SI->getPointerOperand(), SE);
336-
337-
if (NewAlignment > *SI->getAlign()) {
338+
Align OldAlignment = DL.getValueOrABITypeAlignment(
339+
SI->getAlign(), SI->getOperand(0)->getType());
340+
if (NewAlignment > OldAlignment) {
338341
SI->setAlignment(NewAlignment);
339342
++NumStoreAlignChanged;
340343
}

‎llvm/lib/Transforms/Scalar/GVNHoist.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -890,8 +890,8 @@ class GVNHoist {
890890

891891
void updateAlignment(Instruction *I, Instruction *Repl) {
892892
if (auto *ReplacementLoad = dyn_cast<LoadInst>(Repl)) {
893-
ReplacementLoad->setAlignment(MaybeAlign(std::min(
894-
ReplacementLoad->getAlignment(), cast<LoadInst>(I)->getAlignment())));
893+
ReplacementLoad->setAlignment(
894+
std::min(ReplacementLoad->getAlign(), cast<LoadInst>(I)->getAlign()));
895895
++NumLoadsRemoved;
896896
} else if (auto *ReplacementStore = dyn_cast<StoreInst>(Repl)) {
897897
ReplacementStore->setAlignment(

0 commit comments

Comments
 (0)