This is an archive of the discontinued LLVM Phabricator instance.

Driver: hoist the `wchar_t` handling to the driver
ClosedPublic

Authored by compnerd on Sep 14 2017, 11:20 PM.

Details

Reviewers
rnk
MatzeB
Summary

Move the logic for determining the wchar_t type information into the
driver. Rather than passing the single bit of information of
-fshort-wchar indicate to the frontend the desired type of wchar_t
through a new -cc1 option of -fwchar-type and indicate the
signedness through -f{,no-}signed-wchar. This replicates the current
logic which was spread throughout Basic into the
RenderCharacterOptions.

Most of the changes to the tests are to ensure that the frontend uses
the correct type. Add a new test set under test/Driver/wchar_t.c to
ensure that we calculate the proper types for the various cases.

Diff Detail

Repository
rL LLVM

Event Timeline

compnerd created this revision.Sep 14 2017, 11:20 PM
rnk edited edge metadata.Sep 15 2017, 9:20 AM

I do remember recommending this approach over IRC, but I thought we concluded that we should leave all the defaults in lib/Basic/Targets/ and make the -cc1 -fwchar-type= and -f[no-]signed-wchar overrides that affected all targets equally. That would avoid the need for these test updates, anyway.

We could leave the defaults there as they stand, and only have the new flags alter the default. However, it seems that just paying the cost of adjusting the tests once isn't too bad. To me, it seems that having one instance of the horrible logic for determining the type of wchar_t is better than having two copies which can diverge slightly. My understanding was that we determined that it was better to just pay this test adjustment cost, since it would treat all the targets the same.

rnk added a reviewer: MatzeB.Sep 15 2017, 9:39 AM
rnk added inline comments.Sep 15 2017, 9:43 AM
lib/CodeGen/CodeGenModule.cpp
477

@MatzeB ptal

lib/Driver/ToolChains/Clang.cpp
2659

This target detection logic is insanely complicated, and I'm not convinced it's simpler than the Basic/Targets/ delegation strategy.

I can go either way on putting this info in the Driver or Basic, but this code needs to be simpler. Surely a switch could help here?

compnerd added inline comments.Sep 15 2017, 2:05 PM
lib/Driver/ToolChains/Clang.cpp
2659

I like the idea of the switch, I'll try to convert this. In my defense, this was trying to be a translation of the original conditions in Basic. Doesn't mean that I like this or don't want this to converted into something saner.

If we put this into Basic, we get back the 3-bits of information ({char, short, int} * {signed, unsigned}) into 1-bit :-p. This was a pretty painful change, but I think that it does result in a simpler flow for the frontend.

compnerd updated this revision to Diff 115510.Sep 15 2017, 3:29 PM

Try to clarify the logic for APCS ABI.

MatzeB added inline comments.Sep 15 2017, 5:02 PM
lib/CodeGen/CodeGenModule.cpp
477

Can you find a new place for this assert()? Please do not just remove it!

For the backstory: Unfortunately I had to duplicate the wchar decision logic inside llvm (TargetLibraryInfoImpl::getTargetWCharSize() for cases where we just have the target triple available but need to know the size of wchar_t using library function. This means the logic in LLVM needs to be updated when support for new platforms is added but for people adding platform support it will not be obvious that they have the change LLVM/TargetLibraryInfo as well unless an assert() point them to there being a mismatch.

compnerd added inline comments.Sep 15 2017, 5:58 PM
lib/CodeGen/CodeGenModule.cpp
477

Sure, I'll try to see if I can find a suitable place or adjustment of it. However, one thing to note is that the frontend does actually embed that into the IR metadata ("wchar_size").

compnerd added inline comments.Sep 16 2017, 2:48 PM
lib/CodeGen/CodeGenModule.cpp
477

I think that if we try to retain this assertion, we need to update all the tests to ensure that they pass the arguments for selecting the wchar_t type. The entire problem is that the backend view of this cannot correlate with what the user specified without passing it back to it. The "wchar_size" metadata does exactly that. Using that to perform the validation for the library function call.

MatzeB added inline comments.Sep 26 2017, 2:37 PM
lib/CodeGen/CodeGenModule.cpp
477

This has been resolved with r314187 that made the assertion obsolete.

compnerd updated this revision to Diff 117832.Oct 5 2017, 9:00 AM

Moves the logic back into Basic. The flags are now optional, but controlled by the driver. The test adjustments are to map the old -fshort-wchar to -fwchar-type=short -fno-signed-wchar for the most part, there is one case where we were checking that we were passing -fshort-wchar through to the frontend, which we no longer do.

rnk added inline comments.Oct 5 2017, 11:45 AM
lib/Basic/TargetInfo.cpp
29

How is this better than what we had before? It's totally inconsistent with our existing strategy for figuring out type widths and sizes. Our current approach can be extended for new targets, this requires modifying shared TargetInfo code. This refactoring *should* be NFC anyway, so if we did want to do it, we'd want to split it out.

compnerd added inline comments.Oct 5 2017, 12:03 PM
lib/Basic/TargetInfo.cpp
29

The previous thing was split across and was fairly difficult to identify what was going on. I tend to think that this makes it easier to identify what is going on. However, if you have a strong opinion on this, Im willing to switch it back (though, possibly retain some of the adjustments to make it easier to follow).

rnk added inline comments.Oct 5 2017, 3:23 PM
lib/Basic/TargetInfo.cpp
29

I do, I want to see the minimal functional change. It'll make it easier to spot awkward places where we duplicate logic.

compnerd updated this revision to Diff 118085.Oct 6 2017, 2:43 PM

Split the defaulting back to all the various targets.

rnk accepted this revision.Oct 6 2017, 3:44 PM

Looks good with nits

lib/Basic/Targets/AArch64.cpp
47–51

I felt like this was clearer the way it was before, and we're already checking for the BSDs above.

160–161

This is correct because we compute macros after we apply the flag override, right?

This revision is now accepted and ready to land.Oct 6 2017, 3:44 PM
compnerd added inline comments.Oct 6 2017, 4:10 PM
lib/Basic/Targets/AArch64.cpp
47–51

Okay, I'll swap it back.

160–161

Correct :-)

compnerd closed this revision.Oct 6 2017, 4:18 PM

SVN r315126