Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Differential D148477

[mlir][math] Properly disable ctlz conversion in MathToFuncs.
ClosedPublic
Actions

Authored by vzakhari on Apr 16 2023, 1:15 PM.

Details

Reviewers
j2kun
mehdi_amini
PeteSteinfeld
Commits
rGce47090d00cb: [mlir][math] Properly disable ctlz conversion in MathToFuncs.
Summary

This fixes issues caused by D146261.

Diff Detail

Event Timeline

vzakhari created this revision.Apr 16 2023, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 1:15 PM
Herald added subscribers: bviyer, Moerafaat, zero9178 and 22 others. · View Herald Transcript
vzakhari requested review of this revision.Apr 16 2023, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2023, 1:15 PM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
Harbormaster completed remote builds in B225964: Diff 514047.Apr 16 2023, 1:48 PM
mehdi_amini accepted this revision.Apr 16 2023, 3:43 PM

Thanks!

mlir/test/Conversion/MathToFuncs/ctlz.mlir
2

We could have added the test to the other file that already run without the option enabled.

This revision is now accepted and ready to land.Apr 16 2023, 3:43 PM
vzakhari added inline comments.Apr 16 2023, 3:49 PM
mlir/test/Conversion/MathToFuncs/ctlz.mlir
2

Thanks for the review. I do not think there is another test for math.ctlz that enables convert-math-to-funcs converter explicitly. @j2kun, can you please follow up on this comment?

I will merge it as-is shortly.

Closed by commit rGce47090d00cb: [mlir][math] Properly disable ctlz conversion in MathToFuncs. (authored by vzakhari). · Explain WhyApr 16 2023, 3:54 PM
This revision was automatically updated to reflect the committed changes.
vzakhari added a commit: rGce47090d00cb: [mlir][math] Properly disable ctlz conversion in MathToFuncs..
PeteSteinfeld added a comment.Apr 16 2023, 4:25 PM

Thanks, Slava! This passes all of my tests.

j2kun added a comment.Apr 17 2023, 10:00 AM

What exactly is the followup here? I see the extra test added that ensures ctlz is not lowered when the flag is disabled. Is another test helpful?

Revision Contents
Changeset List

PathSize
mlir/
lib/
Conversion/
MathToFuncs/
5 lines
test/
Conversion/
MathToFuncs/
3 lines

Diff 514056

View Options

mlir/lib/Conversion/MathToFuncs/MathToFuncs.cpp

Loading...
View Options

mlir/test/Conversion/MathToFuncs/ctlz.mlir

Loading...