Page MenuHomePhabricator

[Flang][Driver]Add datalayout before doing LLVM-IR transformation
ClosedPublic

Authored by Leporacanthicus on Sep 9 2022, 3:56 AM.

Details

Summary

The earlier available datalyaout allows MLIR to LLVM-IR transformation
to use the datalayout for decisions, such as comparing sizes for
different types of integers.

This should solve https://github.com/llvm/llvm-project/issues/57230

Diff Detail

Event Timeline

Leporacanthicus created this revision.Sep 9 2022, 3:56 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Leporacanthicus requested review of this revision.Sep 9 2022, 3:56 AM

Thanks for implementing this @MatsPetersson !

Could add a test to demonstrate the behavior when the input MLIR module does and does not define the data layout?

flang/lib/Frontend/FrontendActions.cpp
590

The comment in FrontendActions.h is a bit confusing:

/// Sets up LLVM's TargetMachine, configures llvmModule accordingly.

With your changes, llvmModule may or may not be configured. And even without your changes, the data layout (extracted from the target machine), is set after the call to setUpTargetMachine (so the LLVM module is not configured accordingly).

I would update this method to focus on "setting up the target machine". And the bit concerned with setting up the triple should probably be extracted.

flang/test/Driver/pic-flags.f90
2

Unrelated change

17

Unrelated change

flang/unittests/Frontend/FrontendActionTest.cpp
181–185

Why is this suddenly needed?

vzakhari added inline comments.Sep 13 2022, 6:18 PM
flang/lib/Frontend/FrontendActions.cpp
719

I am not sure if we can use string representation of LLVM data layout as MLIR data layout. I would rather follow the Polygeist way and translate LLVM data layout using this method.

753

Do we need this, if MLIR module already has proper data layout? Shouldn't it be translated into LLVM data layout during translation to LLVM IR dialect?

Leporacanthicus marked an inline comment as done.Sep 14 2022, 8:18 AM
Leporacanthicus added inline comments.
flang/lib/Frontend/FrontendActions.cpp
719

Good point. I am trying to test this thoroughly, but I have been busy with other things.

753

Yes. if someone feeds flang-new with a .ll file, it will need to set the DL for LLVM to use later. Because we never created an MLIR module. Hence the check for "is it default" before setting it - we only set it if it wasn't already set.

flang/test/Driver/pic-flags.f90
2

No. We need all the targets that the test runs to to be able to run - now that we set the DL earlier, it fails, where before it didn't.

flang/unittests/Frontend/FrontendActionTest.cpp
181–185

Because when we set the DataLayout earlier, the TargetMachine needs to exist for it to be able to set. It took me a while to understand, but it was clearly the problem, as I discovered once I realized the difference between this test [before these lines were added] and the next one below. We tried to get the TargetMachine, and the LLVM infrastructure has an empty list of target machines, retuns NULL [or end() from some storage]

Previously, we only set DataLayout before turning the LLVM-IR into machine-code, which doesn't happen during this test.

Thanks for the clarifications, Mats! I see that with this change you are also making sure that the datalayout is set before the -emit-llvm actions ends. I think that technically that's not required for this change to work - or am I still missing something? In either case, it's not too much noise and we can keep here. But it would be good to point out in the commit message that with this change, flang-new -emit-llvm -S file.f90 will generate an LLVM IR file with correct datalayout present. I hope that I got it right :)

flang/lib/Frontend/FrontendActions.cpp
753

@Leporacanthicus This makes sense, but the name of method that you are using here does not convey that (not your fault!) :) If this was something like this then the intent would be clearer:

if (!llvmModule->getDataLayout())

So, I suggest adding a comment to explain that isDefault() is equivalent to "there is no data layout yet" :)

Updates based on review comments:

  • Additional MLIR DataLayout.
  • Add check lines for datalayout in the MLIR and LLVM output
awarzynski added inline comments.Oct 6 2022, 7:31 AM
flang/lib/Frontend/FrontendActions.cpp
170–174

Added some suggestion inline (spiting the logic a bit and adding missing comment)

In general, this feels a bit weird. If the lowering bridge provides the MLIR module then it should also configure it correctly, right? i.e. set-up the datalayout. I don't mind this being added here, just thinking about the overall design.

586–587

Why is it not safe to reset it?

I think that it's a good idea to only set it once per compilation. Unless there's a good reasons to allow it multiple times, but I'm not aware of a case requiring that.

754

What happens when consuming an LLVM IR module? In particular, there are 4 scenarios:

  • input module contains DL, compiler invocation contains --target=<target>
  • input module contains DL, compiler invocation does not contain --target=<target>
  • input module does not contain DL, compiler invocation contains --target=<target>
  • input module does not contain DL, compiler invocation does not contain --target=<target>

Is the behavior "before" and "after" this change identical? And is it consistent with Clang? I feel that this particular change might have changed the behavior of flang-new.

Updates per review comments and local testing.

Mainly, set DataLayout for more conditions, rather than skipping the setting.

flang/lib/Frontend/FrontendActions.cpp
170–174

I looked at this, but it's rather complicated, there isn't enough infrastructure in place to do this.

I plan on raising a ticket for "MLIR support for DataLayout is lacking" (or something like that), but I'd like to have this done first.

586–587

Safe in this case is "doesn't unnecessarily set things that we've already set". I'll reword.

Anyway, turns out we want to set it every time, so no longer there... :)

719

So, it turns out we need both the translation and setting the LLVM datalayout, otherwise we can't "recover" the original layout.

As mentioned above, I intend to raise a ticket for "insufficient DL support in MLIR".

754

Comparing with clang, it behaves the same (except for using SSE instructions in clang, and not in Flang - I don't believe that's to do with my changes tho'). I would like to know what it is, actually, since it probably wouldn't hurt to have in Flang - on the other hand, x86-32 isn't a key target afaik.

Either way, it matches the previous implementation after I tested things.

vzakhari added inline comments.Oct 20 2022, 12:25 PM
flang/lib/Frontend/FrontendActions.cpp
762

typo: teh

763

typo: differen

vzakhari accepted this revision.Oct 24 2022, 8:30 PM

I accept this (module the typos), but please make sure @awarzynski is okay with the changes. Thank you!

This revision is now accepted and ready to land.Oct 24 2022, 8:30 PM
awarzynski added inline comments.Oct 25 2022, 1:44 AM
flang/lib/Frontend/FrontendActions.cpp
139

Do you need this temporary?

170–174

I looked at this, but it's rather complicated, there isn't enough infrastructure in place to do this.

This is only addressing my 2nd comment in the box above (starting with "In general"). How about the earlier bit? Amongst other things, this comment is incomplete:

// Fetch module from lb, so we can set

I've also made some suggestions inline.

754

Why? :) This comment currently repeats "what" the code does. But it's hard to understand the "why" behind it. Ultimately, we need clear documentation for where the triple comes from.

IIRC, tm is a member of CompilerInvocation. So, tm contains the triple specified on the command line (or the default triple if no triple is specified). And in this block, you are making sure that it's the set-up from CompilerInvocaiton is used to set-up the triple.

This is, IMHO, key. The triple can come from 2 places (3 if you count MLIR modules) and I would like for the code to clearly document this.

flang/test/Driver/pic-flags.f90
2

Unrelated change, please extract to a separate patch.

Update comments (clarify/reword, fix typos) and rebase.

awarzynski accepted this revision.Wed, Nov 2, 1:24 PM

Thank you @Leporacanthicus , this has been a tricky one! LGTM