This is an archive of the discontinued LLVM Phabricator instance.

Add "X / 1.0" to SimplifyFDivInst

Authored by zansari on Dec 8 2016, 11:56 AM.



We support most other simplifications, but not "X / 1.0", for some reason.

This should be legal with and without NaNs.

I also added a case to call SimplifyFDivInst in SimplifyFPBinOp. Again, I'm not sure why this was missing, but it's useful to have for cases where the fast math flags are important for simplification (otherwise, we end up calling SimplifyBinOp which will drop the FMFs by the time it gets back to SimplifyFDivInst).

Diff Detail


Event Timeline

zansari updated this revision to Diff 80797.Dec 8 2016, 11:56 AM
zansari retitled this revision from to Add "X / 1.0" to SimplifyFDivInst.
zansari updated this object.
zansari added reviewers: sanjoy, spatel, majnemer.
zansari added subscribers: DavidKreitzer, llvm-commits.
majnemer added inline comments.Dec 8 2016, 12:30 PM
1–19 ↗(On Diff #80797)

I'd prefer if we replaced this test with once which was more specific in test/Transforms/InstSimplify/fdiv.ll

spatel edited edge metadata.Dec 8 2016, 12:38 PM

I just added a test for this before reading David's comment: rL289098 .

Zia, you can update the CHECK lines there; we shouldn't need a more complicated regression test unless I'm missing something.

I assume it's just oversight that this is missing from InstSimplify. Currently, InstCombine gets it via:
/ CvtFDivConstToReciprocal tries to convert X/C into X*1/C if C not a special
/ FP value and:
/ 1) 1/C is exact, or
/ 2) reciprocal is allowed.
/ If the conversion was successful, the simplified expression "X * 1/C" is
/ returned; otherwise, NULL is returned.

zansari updated this revision to Diff 80814.Dec 8 2016, 1:07 PM
zansari edited edge metadata.

That was timely and convenient, Sanjay.. Thanks :)

Also, thanks David.

Removed my overly complicated test, and "FIXME'ed" Sanjay's test to catch X/1.0.

spatel accepted this revision.Dec 8 2016, 1:30 PM
spatel edited edge metadata.


This revision is now accepted and ready to land.Dec 8 2016, 1:30 PM
This revision was automatically updated to reflect the committed changes.