This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Disallow zexts in ARMCodeGenPrepare
ClosedPublic

Authored by samparker on Aug 9 2018, 9:17 AM.

Details

Summary

Enabling ARMCodeGenPrepare by default caused a whole load of failures. This is due to zexts and truncs not being handled properly. ZExts are messy so it's just easier to disable for now and truncs are allowed only as 'sinks'. I still need to figure out why allowing them as 'sources' causes so many failures. The other main change is that we are explicit in the types that we converting to, it's now always 'TypeSize'. Type support is also now performed while checking for valid opcodes as it unnecessarily complicated having the checks are different stages.

I've moved the tests around too, so we have the zext and truncs in their own file as well as the overflowing opcode tests.

Diff Detail

Event Timeline

samparker created this revision.Aug 9 2018, 9:17 AM
rnk added a comment.Aug 9 2018, 10:37 AM

I'm not familiar enough with the code to stamp it, but thanks for fixing this!

lib/Target/ARM/ARMCodeGenPrepare.cpp
517–518

Commented out code not intended for commit?

test/CodeGen/ARM/arm-cgp-zext-truncs.ll
267–270

I take it this is a reduction of the test case I sent?

samparker added inline comments.Aug 10 2018, 3:12 AM
lib/Target/ARM/ARMCodeGenPrepare.cpp
517–518

ah yep.

test/CodeGen/ARM/arm-cgp-zext-truncs.ll
267–270

yes, its the exact IR just with the function renamed to something half readable.

SjoerdMeijer added inline comments.Aug 10 2018, 3:48 AM
lib/Target/ARM/ARMCodeGenPrepare.cpp
146

This is a nit, but I would appreciate if you can elaborate a little on the definition of "sources" and "sinks". Thus, more in general, elaborate a bit on the terminology, and also the rationale behind it. I mean, for example, I appreciate I can read in the implementation that an argument, load, call, can be a source. But could there be more, and if so, why are you only interested in these? Perhaps a bit of an algorithmic description is nice too and how these sources and sinks fit in to the bigger picture? I appreciate most information is there already, but could be made a bit more explicit, which is why this is a nit.

samparker updated this revision to Diff 160094.Aug 10 2018, 5:50 AM
  • Removed commented out code
  • Added some TODOs
  • Expanded the description of what a source and sink are
SjoerdMeijer accepted this revision.Aug 10 2018, 6:26 AM

These changes look reasonable to me.

The pass is still disabled by default, so this is a good opportunity (before we start thinking about flipping the switch again) to do more testing and get more experience with the pass without annoying others.

lib/Target/ARM/ARMCodeGenPrepare.cpp
495

Some inconsistent terminology here (sources vs. roots), but this can be addressed in follow up patches.

This revision is now accepted and ready to land.Aug 10 2018, 6:26 AM
This revision was automatically updated to reflect the committed changes.