Page MenuHomePhabricator

[flang][fir] Update the code generation test tool
AbandonedPublic

Authored by schweitz on Feb 10 2021, 2:27 PM.

Details

Summary

This patch updates the code generation test tool, which will be used in subsequent patches to test the FIR ops, lowering them to LLVM IR, etc. It also includes a number of files with support code which will be used by code generation as pieces are added in future patches.

Diff Detail

Event Timeline

schweitz created this revision.Feb 10 2021, 2:27 PM
schweitz requested review of this revision.Feb 10 2021, 2:27 PM
Herald added a project: Restricted Project. · View Herald Transcript
mehdi_amini added inline comments.Feb 10 2021, 3:41 PM
flang/include/flang/Optimizer/Support/FIRContext.h
32

I have concerns with such approach that makes the compilation flow very stateful, where things like a Triple can easily be encoded in the IR as an attribute for example.

flang/include/flang/Optimizer/Support/InitFIR.h
13

Seems like all the header guards need an update.

37

There is no loading involved, the function should be renamed here (and the doc updated)

42

Make a note that registration is only ever useful to be able to parse the passes from a textual string (mostly in testing tools) and not to construct a pass pipeline.

69

Do we need two functions to do the same thing? Can you keep one?

flang/lib/Optimizer/Support/FIRContext.cpp
23

Opaque attribute are supposed to be non-semantically impactful I believe and be able to be dropped, Triple does not seems to fit here.

I actually don't know about uniquer and kindmap, but this is making me nervous as well.

schweitz added inline comments.Feb 11 2021, 7:40 AM
flang/include/flang/Optimizer/Support/FIRContext.h
32

Your objections are noted. We'd like to table this for the moment so as to continue upstreaming.

flang/include/flang/Optimizer/Support/InitFIR.h
13

This is unexpected as it follows the same pattern used in previously upstreamed headers. We'll figure out what changed in clang-tidy.

37

Right. The change to registration was made a day or two ago.

69

We'll look into it.

mehdi_amini added inline comments.Feb 11 2021, 1:29 PM
flang/include/flang/Optimizer/Support/FatalError.h
33

This function is just basically "renaming" llvm::report_fatal_error? It isn't clear to me why we wouldn't just use llvm::report_fatal_error?

flang/lib/Optimizer/Support/FIRContext.cpp
23

Seems like your patch update messed up Phabricator comments handling, I can't see your answer here, but I think you wrote:

Your objections are noted. We'd like to table this for the moment so as to continue upstreaming.

The point of reviewing this kind of thing from your out-of-tree prototype to upstream is to address this. We can't just table this.

(Also I don't see anything testing the Opaque attributes right now.)

schweitz abandoned this revision.Feb 11 2021, 3:47 PM