This is an archive of the discontinued LLVM Phabricator instance.

Fix LIT CodeGen/Func-attr.c
ClosedPublic

Authored by zahiraam on Oct 17 2022, 7:46 AM.

Diff Detail

Event Timeline

zahiraam created this revision.Oct 17 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 7:46 AM
zahiraam requested review of this revision.Oct 17 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 7:46 AM

@saugustine A, @jrtc27. Thanks for your comments.
Adding '-target x86_64' is restrictive, but not sure what option to use to express that I want target with zero as default address. @aaron.ballman what do you think?

I've added some more codegen reviewers; I think we usually try to avoid CodeGen tests that rely on specific optimization levels.

erichkeane added inline comments.
clang/test/CodeGen/func-attr.c
1–2

It isn't really appropriate to add an opt-level to the test, and it doesn't really depend on it. I don't really understand @saugustine 's request in the other thread: MOST of the clang-codegen tests aren't supposed to have opt-levels added to them, and will fail because of it.

So I'm unshocked that adding '-O2' to a test would cause it to fail. Clang tests are generally NOT supposed to be run with an opt-setting.

zahiraam added inline comments.Oct 17 2022, 9:24 AM
clang/test/CodeGen/func-attr.c
1–2

I did notice that the there is some extra information generated before the attribute at the function define when Ox (x>0) is used.

erichkeane added inline comments.Oct 17 2022, 9:28 AM
clang/test/CodeGen/func-attr.c
1–2

I don't believe that is unexpected. The wildcard there is likely fine (though you might want to remove a space either before or after it in case that is not generated), or add --disable-llvm-passes to this if the original patch would only really 'work' at certain opt-levels, but having it run opt is going to cause problems downstream, and makes these tests very sensitive to what the middle/backends do.

But running opt on a CodeGen test isn't really appropriate.

zahiraam updated this revision to Diff 468236.Oct 17 2022, 9:39 AM
zahiraam marked an inline comment as done.
efriedma accepted this revision.Oct 17 2022, 11:07 AM
This revision is now accepted and ready to land.Oct 17 2022, 11:07 AM

With these changes in place, this looks fine to me.

The underlying problem here is that portable IR generation has gotten too difficult to test textually. FileCheck mostly works fine for testing IR passes, where we rarely worry about target dependencies and carefully control the input, but there are too many variables in IR generation these days that result in too much variation in the output. The result is that our test suite overwhelmingly tests specific targets instead of striving for portability. Ah well.