This is an archive of the discontinued LLVM Phabricator instance.

[PHITransAddr] Simplify casts in PHITransAddr
ClosedPublic

Authored by kachkov98 on Feb 2 2023, 3:50 AM.

Details

Summary

Try to simplify cast in similar way as for GEP and ADD with constant (e.g. sext/zext + trunc).

Diff Detail

Event Timeline

kachkov98 created this revision.Feb 2 2023, 3:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 3:50 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
kachkov98 requested review of this revision.Feb 2 2023, 3:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 3:50 AM
mkazantsev added inline comments.Feb 2 2023, 4:13 AM
llvm/lib/Analysis/PHITransAddr.cpp
185

I'm fine with isSafeToSpeculativelyExecute part, it seems reasonable. But this doesn't seem to be NFC while whole patch claims it's an NFC. Should it be 2 different patches?

kachkov98 added inline comments.Feb 2 2023, 4:21 AM
llvm/lib/Analysis/PHITransAddr.cpp
185

I agree, after this change it can fold zext/sext + trunc for example (didn't see any changes on test-suite though).

kachkov98 updated this revision to Diff 494266.Feb 2 2023, 5:19 AM

Split patch

kachkov98 retitled this revision from [NFCI] Cleanup processing of casts in PHITransAddr to [PHITransAddr] Simplify casts in PHITransAddr.Feb 2 2023, 5:20 AM
kachkov98 edited the summary of this revision. (Show Details)
mkazantsev added inline comments.Feb 2 2023, 5:29 AM
llvm/lib/Analysis/PHITransAddr.cpp
185

Can you write a test yourself?

mkazantsev requested changes to this revision.Feb 2 2023, 9:56 PM

Please add a test. If you can't figure it out yourself, you can insert an assert and run a big enough tests corps to see if something changed, and then use bugpoint to reduce it. Good luck!

This revision now requires changes to proceed.Feb 2 2023, 9:56 PM
kachkov98 updated this revision to Diff 494565.Feb 3 2023, 2:27 AM

Added synthetic test for simplifyCastInst

Separate patch for removing isSafeToSpeculativelyExecute() checks: https://reviews.llvm.org/D143255

mkazantsev accepted this revision.Feb 3 2023, 3:10 AM

Looks reasonable. I'm not familiar with this code part in general, so please wait some time in case if someone has concerns, but looks pretty straightforward.

This revision is now accepted and ready to land.Feb 3 2023, 3:10 AM

Looks reasonable. I'm not familiar with this code part in general, so please wait some time in case if someone has concerns, but looks pretty straightforward.

Ok, thank you for the reviews!

This revision was landed with ongoing or failed builds.Feb 7 2023, 1:43 AM
This revision was automatically updated to reflect the committed changes.