Page MenuHomePhabricator

add IR flags to MI
ClosedPublic

Authored by mcberg2017 on Sep 6 2018, 10:27 AM.

Details

Summary

Initial support for nsw, nuw and exact flags in MI

Diff Detail

Repository
rL LLVM

Event Timeline

mcberg2017 created this revision.Sep 6 2018, 10:27 AM

I have a test case which will show shortly...

With the appropriate test file...

spatel added a subscriber: nlopes.Sep 7 2018, 8:45 AM

Are there MI transforms that would use these flags?
cc @nlopes - if we add these flags, then we also add the complexity of poison to this layer?

The flags were requested for support in GlobalIsel, where there is a plan to make use of them once they are available. The path through SelectionDAG could also make use of the flags.

Sure, adding nsw/nuw brings in poison. I didn't study all the MI transformations going on, so I can't comment on whether this is a big burden or not, but without such a study introducing poison is dangerous.
SDAG already has nsw/nuw IIRC, though. But I don't think there was a thorough study of the implications at the time either.

All the nsw/nuw context is set above SDAG currently in code that is common to both GlobalIsel and SDAG, meaning adding them to MI should be analogous to SDAG as a pass through and for detection of already derived context for actions and side effects. Currently SDAG does have some DAG combining opts that utilize these flags. MI should too for GlobalIsel to map existing optimizations and functionality, else GlobalIsel will be handicapped. Having said that, if we do due diligence to map SDAG context in GlobalIsel, then there should be no new poison state, only that which currently exists. We should also keep the door open for new SDAG optimizations under nsw/nuw to map into that space to provide parity of support.

spatel accepted this revision.Sep 10 2018, 12:58 PM

Having said that, if we do due diligence to map SDAG context in GlobalIsel, then there should be no new poison state, only that which currently exists.

Agree, but I don't think there has been any thorough study of the poison state in SDAG yet. But it's also true that we don't use those flags for very much, so there isn't much chance for disaster.

This patch is straightforward enough, so I won't impede it, but whoever is working on the translation to GlobalISel/MI transforms should be aware of the danger.

LGTM.

This revision is now accepted and ready to land.Sep 10 2018, 12:58 PM
Closed by commit rL341996: add IR flags to MI (authored by mcberg2017, committed by ). · Explain WhySep 11 2018, 2:37 PM
This revision was automatically updated to reflect the committed changes.