This is an archive of the discontinued LLVM Phabricator instance.

[Alignment][NFC] MaybeAlign in GVNExpression
ClosedPublic

Authored by gchatelet on Sep 23 2019, 8:52 AM.

Details

Summary

This is patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

Diff Detail

Repository
rL LLVM

Event Timeline

gchatelet created this revision.Sep 23 2019, 8:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 8:52 AM
fhahn added a subscriber: fhahn.Sep 23 2019, 9:28 AM
fhahn added inline comments.
llvm/include/llvm/Transforms/Scalar/GVNExpression.h
326 ↗(On Diff #221350)

This is wrapped in namespace llvm {. Is MaybeAlign ambiguous in this context?

gchatelet updated this revision to Diff 221964.Sep 26 2019, 8:44 AM
gchatelet marked an inline comment as done.
  • Address comments
gchatelet marked an inline comment as done.Sep 26 2019, 8:47 AM
gchatelet added inline comments.
llvm/include/llvm/Transforms/Scalar/GVNExpression.h
326 ↗(On Diff #221350)

Indeed, Thx @fhahn.

fhahn added inline comments.Sep 26 2019, 9:07 AM
llvm/lib/Transforms/Scalar/NewGVN.cpp
1335 ↗(On Diff #221964)

llvm:: should not be needed here either. I think in almost all contexts in llvm/, it should not be needed and probably should be removed in the patches already submitted as well.

  • Address comments
gchatelet marked 2 inline comments as done.Sep 27 2019, 1:00 AM
gchatelet added inline comments.
llvm/lib/Transforms/Scalar/NewGVN.cpp
1335 ↗(On Diff #221964)

I'll address this in a separate patch.

courbet accepted this revision.Sep 27 2019, 1:05 AM
This revision is now accepted and ready to land.Sep 27 2019, 1:05 AM
This revision was automatically updated to reflect the committed changes.
gchatelet marked an inline comment as done.