This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Translation] Make commandline option registration optional
ClosedPublic

Authored by rkayaith on Oct 23 2022, 12:43 PM.

Details

Summary

This moves the commandline option registration into its own function, so
that users can register translations without registering the options.

Diff Detail

Event Timeline

rkayaith created this revision.Oct 23 2022, 12:43 PM
rkayaith requested review of this revision.Oct 23 2022, 12:43 PM
Peiming added inline comments.Oct 23 2022, 1:37 PM
mlir/lib/Tools/mlir-translate/Translation.cpp
120–121

Don't you need to test clOptions.isConstructed() before you can use it?

cota accepted this revision.Oct 24 2022, 9:05 AM
cota added inline comments.
mlir/lib/Tools/mlir-translate/Translation.cpp
120–121

Doesn't seem necessary from my reading of the source:

Pointer dereference accessor:

C *operator->() { return &**this; }

which then uses this accessor that constructs the object if necessary:

C &operator*() {
  void *Tmp = Ptr.load(std::memory_order_acquire);
  if (!Tmp)
    RegisterManagedStatic(Creator::call, Deleter::call);

  return *static_cast<C *>(Ptr.load(std::memory_order_relaxed));
}
This revision is now accepted and ready to land.Oct 24 2022, 9:05 AM
Peiming added inline comments.Oct 24 2022, 9:25 AM
mlir/lib/Tools/mlir-translate/Translation.cpp
120–121

Does it mean that operator-> will implicit register the option? if that is the case, it does not solve our problem.

cota requested changes to this revision.Oct 24 2022, 9:30 AM
cota added inline comments.
mlir/lib/Tools/mlir-translate/Translation.cpp
120–121

I talked to Peiming and understood his comment better.
Peiming is right; the problem here is that by dereferencing the options, we are constructing them (and therefore registering them), which defeats the purpose of the patch (i.e. to not register the options). So this cannot proceed as is.

This revision now requires changes to proceed.Oct 24 2022, 9:30 AM
rkayaith added inline comments.Oct 24 2022, 9:30 AM
mlir/lib/Tools/mlir-translate/Translation.cpp
120–121

Yea we want to avoid constructing the cl options object here, so I'll update this to check isConstructed.

rkayaith updated this revision to Diff 470205.Oct 24 2022, 10:19 AM

rebase, avoid construction options

cota accepted this revision.Oct 24 2022, 10:48 AM
This revision is now accepted and ready to land.Oct 24 2022, 10:48 AM