Page MenuHomePhabricator

Verify inferattrs doesn't infer unexpected attributes
ClosedPublic

Authored by probinson on Sep 29 2021, 8:09 AM.

Details

Summary

Add --match-full-lines or {{$}} to ensure that no unexpected
attributes appear at the ends of lines. Account for the cases
where attributes were in fact appearing.

Diff Detail

Event Timeline

probinson requested review of this revision.Sep 29 2021, 8:09 AM
probinson created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 8:09 AM

A bit of yak shaving because I had wanted to add __sqrt*_finite to annotate.ll, but it wasn't doing quite the right thing.

And then I poked around a little in the other tests in this directory. I'm particularly puzzled by dereferenceable.ll and dereferenceable-inseltpoison.ll, which don't appear to do anything other than verify function parameter lists don't get modified; is that really the intent? They seem kind of elaborate for that.

The dereferenceable.ll and dereferenceable-inseltpoison.ll files are also confusing to me. It *sounds* like they're trying to test inferred attributes, but its CHECK* lines would allow any or no attributes on the functions, so it's not actually testing anything attribute-related.

Using --match-full-lines seems fine to me, and this change does look like an improvement.

llvm/test/Transforms/InferFunctionAttrs/annotate.ll
4

Linux isn't nvptx, but it's not passing CHECK-NONVPTX.

Then again, all of the tests pass CHECK, except for the nvptx case. I tested FileCheck, and it seems that CHECK is not implicitly tested when --check-prefixes is specified. So, for nvptx, we were previously only testing __nvvm_reflect and bcmp?

The lines where CHECK is split into CHECK-LINUX and CHECK-NOLINUX are also enabling the test for nvptx.

probinson updated this revision to Diff 384853.Nov 4 2021, 12:58 PM

Sorry it took a while to cycle back to this.

Renamed CHECK-NONVPTX to CHECK-OPEN, because it was used only for the one function declaration, and that way the directive name is not misleading.

The lines where CHECK is split into CHECK-LINUX and CHECK-NOLINUX are also enabling the test for nvptx.

It's true that the nvptx case is now checking more than it used to, but not everything; making it check everything could probably be done, but is it worthwhile? I experimented with passing CHECK to the nvptx case, and it doesn't pass; there would have to be a CHECK-NVPTX line paralleling each CHECK: line, pretty much. Seems like a lot of effort, but is nvptx really that special?
I'll do it if someone says Yes, it's worth the trouble; but unless someone can say that, I'd rather not.

rprichard accepted this revision.Dec 7 2021, 9:39 PM

The lines where CHECK is split into CHECK-LINUX and CHECK-NOLINUX are also enabling the test for nvptx.

It's true that the nvptx case is now checking more than it used to, but not everything; making it check everything could probably be done, but is it worthwhile? I experimented with passing CHECK to the nvptx case, and it doesn't pass; there would have to be a CHECK-NVPTX line paralleling each CHECK: line, pretty much. Seems like a lot of effort, but is nvptx really that special?
I'll do it if someone says Yes, it's worth the trouble; but unless someone can say that, I'd rather not.

Ok, that makes sense to me.

This revision is now accepted and ready to land.Dec 7 2021, 9:39 PM
This revision was automatically updated to reflect the committed changes.