Page MenuHomePhabricator

Check type for forward reference definition
ClosedPublic

Authored by mjp41 on Mon, May 4, 2:51 AM.

Details

Summary

The types of forward references are checked that they match with other
uses, but they do not check they match with the definition.

func @forward_reference_type_check() -> (i8) {
  br ^bb2

^bb1:
  return %1 : i8

^bb2:
  %1 = "bar"() : () -> (f32)
  br ^bb1
}

Would be parsed and the use site of '%1' would be silently changed to
'f32'.

This commit adds a test for this case, and a check during parsing for
the types to match.

Diff Detail

Event Timeline

mjp41 created this revision.Mon, May 4, 2:51 AM
Herald added 1 blocking reviewer(s): rriddle. · View Herald TranscriptMon, May 4, 2:51 AM
Herald added a project: Restricted Project. · View Herald Transcript
mjp41 removed 1 blocking reviewer(s): rriddle.Mon, May 4, 2:52 AM
Herald added 1 blocking reviewer(s): rriddle. · View Herald TranscriptMon, May 4, 2:52 AM
mjp41 updated this revision to Diff 261783.Mon, May 4, 4:58 AM
rriddle accepted this revision.Mon, May 4, 10:23 AM
rriddle added inline comments.
mlir/lib/Parser/Parser.cpp
3630

nit: previously used here with type

This revision is now accepted and ready to land.Mon, May 4, 10:23 AM
mjp41 updated this revision to Diff 262019.Tue, May 5, 1:20 AM

Updated error message inline with feedback.

@rriddle, do you know what's failing in the test build?

The page isn't very clear on what the problem is and the Jenkins URL requests user/passwd.

I've checked clang-format and ninja check-mlir and everything is fine. I was just about to commit...

Are we treating Harbourmaster's failures as blockers, or is that experimental?

cheers,
--renato

I can't tell what is failing from the log, I filed: https://github.com/google/llvm-premerge-checks/issues/176

rengolin closed this revision.Wed, May 6, 6:36 AM

Ok, turns out the failure was a flaky test due to parallelism (CMake deps not right?), but the build finishes and the tests all pass.

I've pushed the change (5010b5b7e6c), as it has been approved and passes all test.

We'll discuss about the issues with the pre-merge tests in separate.

Thanks everyone!

mehdi_amini added a comment.EditedWed, May 6, 11:38 AM

Thanks for landing it @rengolin ; FYI we document now to not use "Patch by" but to use git commit --amend --author="John Doe <jdoe@llvm.org> to have the proper commit author recorded, see https://llvm.org/docs/DeveloperPolicy.html#commit-messages

FYI we document now to not use "Patch by" but to use git commit --amend --author="John Doe <jdoe@llvm.org> to have the proper commit author recorded, see https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Sorry, old habit. Will do that onward. :)