Page MenuHomePhabricator

[SVE] Fix TypeSize warning in RuntimePointerChecking::insert
ClosedPublic

Authored by joechrisellis on Oct 26 2020, 10:24 AM.

Details

Summary

The TypeSize warning would occur because RuntimePointerChecking::insert
was not scalable vector aware. The fix is to use
ScalarEvolution::getSizeOfExpr to grab the size of types.

Diff Detail

Event Timeline

joechrisellis created this revision.Oct 26 2020, 10:24 AM
joechrisellis requested review of this revision.Oct 26 2020, 10:24 AM
fhahn added a subscriber: fhahn.Oct 26 2020, 10:30 AM
fhahn added inline comments.
llvm/test/Analysis/LoopAccessAnalysis/runtime-pointer-checking-insert-typesize.ll
2

Ideally the tests in this directory would only use -loop-accesses -analyze and no transformation passes. Is that possible? Also, is -mattr=+sve needed?

If loop-load-elimination is required, the test should probably go there. Also it might be good to just have all scaleable tests in a single file in the directory>

joechrisellis marked an inline comment as done.

Address @fhahn's suggestions.

llvm/test/Analysis/LoopAccessAnalysis/runtime-pointer-checking-insert-typesize.ll
2

Thanks for the pointers!

Ideally the tests in this directory would only use -loop-accesses -analyze and no transformation passes. Is that possible? Also, is -mattr=+sve needed?

Ah okay -- turns out we can just use -loop-accesses -analyze for the same result. Fixed this.

If loop-load-elimination is required, the test should probably go there.

It's not required. :)

fhahn added inline comments.Wed, Oct 28, 2:10 PM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
227–229

nit: just IdxTy?

229

I think this slightly changes the behavior if store and alloc size are different, is that intentional? (because getSizeOfExpr uses alloc size).

joechrisellis marked an inline comment as done.

Address @fhahn's comments.

  • IntIdxTy -> IdxTy
  • Preserve existing behaviour for non-scalable types.
joechrisellis added inline comments.Tue, Nov 3, 6:44 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
229

Excellent spot -- I have fixed this.

joechrisellis marked an inline comment as done.Tue, Nov 3, 6:44 AM
fhahn added inline comments.Tue, Nov 3, 6:52 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
229

Not sure how hard it would be to come up with a test, but it would probably be good to have one

joechrisellis added inline comments.Fri, Nov 13, 1:16 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
229

Hi @fhahn, sorry for the lag in replying here!

I tried to make a test for this but wasn't able to come up with anything that defends against the different behaviour when alloc size != store size. I think I will leave this out from the patch, if that is okay with you? Otherwise, it would be useful to hear your ideas on how we could create such a test. :)

@fhahn Hi! Do you have any objections to landing this as-is without the additional test? Alternatively, any recommendations on how to test this? 😄

fhahn added a comment.Mon, Nov 23, 1:43 PM

@fhahn Hi! Do you have any objections to landing this as-is without the additional test? Alternatively, any recommendations on how to test this? 😄

I think you would need to use element types with different store & alloca sizes, like i19 (see https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/DataLayout.h#L446 for some examples). You should be able to adjust a test with runtime checks (e.g. llvm/test/Analysis/LoopAccessAnalysis/memcheck-off-by-one-error.ll) and update it to use a type like i19 to get such a test. If you decide to use llvm/test/Analysis/LoopAccessAnalysis/memcheck-off-by-one-error.ll, I think some of the casts in the loop body could be stripped.

llvm/test/Analysis/LoopAccessAnalysis/runtime-pointer-checking-insert-typesize.ll
2

If loop-load-elimination is required, the test should probably go there.

It's not required. :)

Looks like there's another test that runs -loop-load-elim: llvm/test/Analysis/LoopAccessAnalysis/gep-induction-operand-typesize-warning.ll. Could this test also be updated?

Add test to defend against accidentally using alloc size instead of store size.

Thank you @fhahn, that comment was super helpful. I've opted to create a new test here because I didn't want to change the relationship between the IR and the C++ at the top of memcheck-off-by-on-error.ll. If you think this is the wrong thing to do, just let me know. 😄

fhahn accepted this revision.Tue, Nov 24, 10:32 AM

LGTM with the comment adjustment, thanks!

llvm/lib/Analysis/ScalarEvolution.cpp
3685

if you are touching the comment lines, could. you also re-flow the comment?

Also, this comment should be moved to the getConstant call, right? Because for scalable vectors we create a constant expression.

This revision is now accepted and ready to land.Tue, Nov 24, 10:32 AM
joechrisellis marked an inline comment as done.

Move and re-flow comment lines.

This revision was landed with ongoing or failed builds.Wed, Nov 25, 8:59 AM
This revision was automatically updated to reflect the committed changes.