Page MenuHomePhabricator

[MLIR][LLVM] Add error if using incorrect attribute type for specifying LLVM linkage
ClosedPublic

Authored by wsmoses on Sep 27 2021, 9:56 AM.

Details

Diff Detail

Event Timeline

wsmoses created this revision.Sep 27 2021, 9:56 AM
wsmoses requested review of this revision.Sep 27 2021, 9:56 AM
mehdi_amini accepted this revision.Sep 27 2021, 10:04 AM

LG with comment!

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
269–276

In general we avoid isa->cast

linkage = funcOp->getAttr("llvm.linkage")
              .dyn_cast<mlir::LLVM::LinkageAttr>()
              .getLinkage();
if (!linkage) {
  funcOp->emitError() << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
  return nullptr;
}
271

Seems above 80chars to me, can you double check this is clang-formatted?

mlir/test/Conversion/StandardToLLVM/convert-funcs.mlir
43

(Spurious change)

This revision is now accepted and ready to land.Sep 27 2021, 10:04 AM
bondhugula added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
269–276

You are using the result of a dyn_cast unchecked here.

wsmoses updated this revision to Diff 375321.Sep 27 2021, 10:11 AM

Address comments

mehdi_amini added inline comments.Sep 27 2021, 10:15 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
269–276

Grahh you're right, I went too fast here.
The alternative aren't as enticing though:

auto linkageAttr = funcOp->getAttr("llvm.linkage").dyn_cast<mlir::LLVM::LinkageAttr>();
if (! linkageAttr) {
  funcOp->emitError() << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
  return nullptr;
}
linkage = linkageAttr.getLinkage();

or:

if (auto linkageAttr = funcOp->getAttr("llvm.linkage").dyn_cast<mlir::LLVM::LinkageAttr>()) {
    linkage = linkageAttr.getLinkage();
} else {
  funcOp->emitError() << "Contains llvm.linkage attribute not of type LLVM::LinkageAttr";
  return nullptr;
}
269–276

I see that Billy already adopted the first one, LGTM

This revision was landed with ongoing or failed builds.Sep 27 2021, 10:24 AM
This revision was automatically updated to reflect the committed changes.
wsmoses marked 5 inline comments as done.