This is an archive of the discontinued LLVM Phabricator instance.

[Alignment][NFC] Value::getPointerAlignment returns MaybeAlign
ClosedPublic

Authored by gchatelet on Oct 3 2019, 8:30 AM.

Details

Diff Detail

Event Timeline

gchatelet created this revision.Oct 3 2019, 8:30 AM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert added inline comments.Oct 3 2019, 1:37 PM
llvm/lib/Analysis/Loads.cpp
48

While I'm all for making the "getAlign" function explicit eventually, I think it would be good to keep it as is for this patch as there doesn't seem to be a reason to do this here.

fwiw: I'm behind but eventually I was going to refactor this: D66618

gchatelet marked an inline comment as done.Oct 9 2019, 1:12 AM
gchatelet added inline comments.
llvm/lib/Analysis/Loads.cpp
48

Which function are you talking about exactly? I don't see a getAlign function.

gchatelet marked an inline comment as done.Oct 10 2019, 1:43 AM
gchatelet added inline comments.
llvm/lib/Analysis/Loads.cpp
48

@jdoerfert

Ha I suppose you're talking about getBaseAlign.
I was looking at D66618 and I believe the code will be easier to modify once it is moved to llvm::Align (I'm biased obviously).
Also the getPointerAlignment function has already been massaged in D67918 to prevent reuse of the Align variable and make logic more local rather than spanning the whole function.
I believe you'll have to rebase D66618 and you'll have conflicts there already.

On my side I'd need to move forward with the introduction of llvm::Align, I can make progress on other fronts but I'll have to update Value and Load eventually.
Please let me know what are your plans and schedule with D66618 so I can plan accordingly. Thx!

I don't see any progress on D66618 so I'll proceed with the review of this patch and submit if you don't mind @jdoerfert

courbet accepted this revision.Oct 15 2019, 6:38 AM
courbet added inline comments.
llvm/lib/Analysis/Loads.cpp
35

just None.

llvm/lib/IR/Value.cpp
687–688

remove one MaybeAlign

713–714

ditto

721–725

ditto

732

just None.

This revision is now accepted and ready to land.Oct 15 2019, 6:38 AM
gchatelet updated this revision to Diff 225023.Oct 15 2019, 6:47 AM
gchatelet marked 6 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.