efriedma marked an inline comment as done.
efriedma added inline comments.
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:60
+ } else {
emitError(loc) << "expected sequential LLVM types wrapping a scalar";
return nullptr;
ftynse wrote:
>
ftynse added a comment.
LGTM for MLIR part.
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:60
+ } else {
emitError(loc) << "expected sequential LLVM types wrapping a scalar";
return nullptr;
Nit: can we update the error message not to
This revision was automatically updated to reflect the committed changes.
Closed by commit rG68b03aee1a15: Remove SequentialType from the type heirarchy.
(authored by efriedma).
Changed prior to commit:
https://reviews.llvm.org/D75661?vs=252659=255546#toc
Repository:
rG LLVM Github Monorepo
ctetreau added a comment.
Herald added a subscriber: grosul1.
What's the status on this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75661/new/
https://reviews.llvm.org/D75661
___
cfe-commits
ctetreau added inline comments.
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:81
static llvm::Type *getInnermostElementType(llvm::Type *type) {
- while (isa(type))
-type = type->getSequentialElementType();
- return type;
+ do {
+if (auto *arrayTy =
efriedma updated this revision to Diff 252659.
efriedma added a comment.
Herald added subscribers: Joonsoo, liufengdb, lucyrfox, mgester, arpith-jacob,
csigg, nicolasvasilache, antiagainst, shauheen, burmako, jpienaar, rriddle,
mehdi_amini.
Fix bug in GlobalOpt. Add fixes for MLIR.
efriedma updated this revision to Diff 252645.
efriedma added a comment.
Address lint comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75661/new/
https://reviews.llvm.org/D75661
Files:
clang/lib/CodeGen/CGExprConstant.cpp
ctetreau added a comment.
I'd like the see the clang-format complaints actually addressed, but don't
bother unless you need to upload another patch for some other reason anyways.
That aside, LGTM. This has been looked at by 3 people (plus yourself). So far,
nobody has had any serious
efriedma updated this revision to Diff 252432.
efriedma edited the summary of this revision.
efriedma added a comment.
Rebased. (CGDecl.cpp changes landed separately.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75661/new/
ctetreau added inline comments.
Comment at: clang/lib/CodeGen/CGDecl.cpp:1078
}
+ // FIXME: Do we need to handle tail padding in vectors?
return constant;
efriedma wrote:
> ctetreau wrote:
> > The fact that you have to ask this question tells me that you
efriedma marked 2 inline comments as done.
efriedma added inline comments.
Comment at: clang/lib/CodeGen/CGDecl.cpp:1059
+llvm::Type *ElemTy = ArrayTy->getElementType();
+bool ZeroInitializer = constant->isNullValue();
llvm::Constant *OpValue, *PaddedOp;
ctetreau added inline comments.
Herald added a reviewer: aartbik.
Comment at: clang/lib/CodeGen/CGDecl.cpp:1059
+llvm::Type *ElemTy = ArrayTy->getElementType();
+bool ZeroInitializer = constant->isNullValue();
llvm::Constant *OpValue, *PaddedOp;
I
sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.
Thank you for this patch! Personally I find the code more readable without the
SequentialType abstraction and the use of the GEP interface
(getTypeAtIndex(Type, Idx)) you added in
efriedma updated this revision to Diff 251218.
efriedma added a comment.
I went through the exercise of completely removing the notion of a "sequential"
type; the result comes out something like this.
Broadly, the places that might be able to use some notion of "sequential"
mostly fall into
ctetreau added a comment.
> I'm concerned though about having getSequentialElementType and
> getSequentialNumElements apply to different sets of types. That's bound to
> lead to confusion which causes bugs. Can we have one name for all types that
> are homogenous sequences of the same element
efriedma added a comment.
I guess the API names could be a bit confusing, yes. Any suggestions for
better names?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75661/new/
https://reviews.llvm.org/D75661
nhaehnle added a comment.
The AMDGPU changes LGTM, modulo a stylistic nitpick.
As for the overall change, I'm perfectly fine with the general direction of
SequentialType, though I don't know where we stand in terms of enough people
having had a chance to look at it.
I'm concerned though about
efriedma updated this revision to Diff 250928.
efriedma added a comment.
Minor fix to AMDGPU changes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75661/new/
https://reviews.llvm.org/D75661
Files:
clang/lib/CodeGen/CGDecl.cpp
efriedma marked an inline comment as done.
efriedma added a comment.
I looked at getting rid of getSequentialNumElements/getSequentialElementType,
but I'm not sure that would actually make the code more clear. I avoided them
in cases where they clearly weren't helpful.
efriedma updated this revision to Diff 250910.
efriedma added a comment.
Rebase. Fix a test failure. A few other minor tweaks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75661/new/
https://reviews.llvm.org/D75661
Files:
efriedma created this revision.
Herald added subscribers: cfe-commits, kerbowa, hiraditya, nhaehnle, jvesely,
arsenm.
Herald added projects: clang, LLVM.
efriedma added a parent revision: D75660: Remove CompositeType class.
This is a bit more dubious than removing CompositeType, given the number
21 matches
Mail list logo
