This is an archive of the discontinued LLVM Phabricator instance.

Make IRBuilder automatically set alignment on load/store/alloca.
ClosedPublic

Authored by efriedma on Apr 12 2020, 5:37 PM.

Details

Summary

This is equivalent in terms of LLVM IR semantics, but we want to transition away from using MaybeAlign to represent the alignment of these instructions.

Diff Detail

Event Timeline

efriedma created this revision.Apr 12 2020, 5:37 PM
Herald added a project: Restricted Project. · View Herald Transcript

Thanks, this is indeed much better and the direction again in-line with what i've raised in some attributor patch.
LG to me in general, but please wait for someone else to take a look, too.

I think this is fine. Two comments to consider below.

llvm/include/llvm/IR/IRBuilder.h
1597

Can't we just pawn of the alignment stuff to CreateAlignedLoad? We cal lit with 0 here and we determine an alignment there anyway?
Nit: BB->getModule() should work.

1617

Same as for the load.

efriedma marked an inline comment as done.Apr 13 2020, 10:11 AM
efriedma added inline comments.
llvm/include/llvm/IR/IRBuilder.h
1597

I was thinking we might want to switch CreateAlignedLoad to take an Align instead of a MaybeAlign: I'm not sure there's any good reason to use a MaybeAlign once we clean up the getters on load/store/alloca. But I guess we can do that later, if it turns out to actually be feasible; I'll change it.

efriedma updated this revision to Diff 257059.Apr 13 2020, 12:12 PM

Address review comments

efriedma planned changes to this revision.Apr 13 2020, 12:19 PM

Err, didn't quite address all the review comments. New version soon.

efriedma updated this revision to Diff 257063.Apr 13 2020, 12:30 PM

Actually address all the review comments. Fix CreateAlloca to use the pref alignment instead of the ABI alignment, like instcombine and selectiondag.

This revision is now accepted and ready to land.Apr 13 2020, 1:22 PM
This revision was automatically updated to reflect the committed changes.
bondhugula added a subscriber: bondhugula.EditedApr 13 2020, 9:16 PM

This revision has broken an MLIR test case that tests conversion to LLVM IR (test/Target/llvmir.mlir - an llvm.alloca in MLIR with its alignment attribute set to 0 now converts into an LLVM IR alloca with alignment automatically set to 4 - I think the fix is trivial; the check there is expecting to see no align attribute attached to the resulting alloca). Please build with -DLLVM_ENABLE_PROJECTS=mlir to reproduce, but revisions submitted via arcanist will already show the failed test cases.

This revision has broken an MLIR test case that test conversion to LLVM IR (test/Target/llvmir.mlir - an llvm.alloca in MLIR with its alignment attribute set to 0 now converts into an LLVM IR alloca with alignment automatically set to 4 - I think the fix is trivial; the check there is expecting to see no align attribute attached to the resulting alloca). Please build with -DLLVM_ENABLE_PROJECTS=mlir to reproduce, but revisions submitted via arcanist will already show the failed test cases.

I've gone ahead and fixed it in 3c30e17346a324029bbb59abee9f7e6a9265dfc7.

That fix looks right; thanks.