Page MenuHomePhabricator

[InstCombine][ConstantFolding] Make ConstantFoldLoadThroughBitcast TypeSize-aware
ClosedPublic

Authored by peterwaller-arm on Oct 20 2021, 8:23 AM.

Details

Summary

The newly added test previously caused the compiler to fail an
assertion. It looks like a strightforward TypeSize upgrade.

Diff Detail

Event Timeline

peterwaller-arm requested review of this revision.Oct 20 2021, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2021, 8:23 AM

Hi @peterwaller-arm, the patch looks good to me! I wonder if it's worth renaming the test to vscale_load.ll so that it's less specific? That way we can potentially more load related fold tests here in future as well.

llvm/test/Transforms/InstCombine/vscale_load_bitcast.ll
6 ↗(On Diff #380964)

I guess this fold is happening because effectively i1 is being promoted to i8? Otherwise the minimum size of <vscale x 16 x i1> would be just 2 bytes, i.e. less than than <8 x i8>.

llvm/lib/Analysis/ConstantFolding.cpp
357

You cannot use TypeSize in this way. Essentially TypeSize can only prove a positive not a negative.

This code block is effectively saying "the following code is only safe when SrcSize >= DestSize and you must exit when this is not true". Whereas you've changed it to be "if and only if I can prove SrcSize < DestSize shall I exit".

So I believe the correct change is rather

if (!TypeSize::isKnownGE(SrcSize, DestSize))`
llvm/test/Transforms/InstCombine/vscale_load_bitcast.ll
6 ↗(On Diff #380964)

This is likely due to the issue above but at the same time the test is relying on undefined behaviour. You either want to use a smaller fixed length vector or perhaps add a vscale_range so the load does overlap the store.

Can you also add a negative test for the case where the load & store must remain. I guess if you want to tick all the boxes you could also add a test where the store remains but the load is removed.

  • Fix typesize query to use !isKnownGE.
  • Add a negative test.
peterwaller-arm marked 3 inline comments as done.Oct 21 2021, 3:24 AM
peterwaller-arm added inline comments.
llvm/test/Transforms/InstCombine/vscale_load_bitcast.ll
6 ↗(On Diff #380964)

Right. I've added a negative test and fixed up the size query, thanks both for your attention to detail!

I considered your comment about vscale_range but:

  1. I don't see a straightforward route to get at it without propagating the function attributes through the constant folding chain. Maybe that's the right thing to do? Or is there a way to take Constant* and a Type*, and ask "what vscale_range is in effect?". I'm guessing not since these aren't in principle bound to functions. I don't want to fiddle with the function signature without a positive steer from someone that it's the right thing to do.
  1. What is the best way to make isKnown* queries in the presence of a vscale_range attribute?

My vscale_range comment is not concerning the implementation but rather about ensuring the ll tests do not contain potentially undefined behaviour. You can ignore this because I've commented below with an alternative that is simpler.

llvm/test/Transforms/InstCombine/vscale_load.ll
28

This is potentially undefined behaviour (the load can read beyond the alloca). A better option is for both tests to replace the alloca with a pointer parameter. That way the bounds of the storage are unknown and it becomes only about the store and load.

peterwaller-arm marked an inline comment as done.
  • Update tests per review comments, use ptr param instead of alloca.
peterwaller-arm marked an inline comment as done.Oct 26 2021, 2:02 AM
paulwalker-arm accepted this revision.Oct 27 2021, 5:43 AM
This revision is now accepted and ready to land.Oct 27 2021, 5:43 AM
This revision was landed with ongoing or failed builds.Oct 28 2021, 5:26 AM
This revision was automatically updated to reflect the committed changes.