[ARM] handle promotion of zero sized constants.
ClosedPublic

Authored by TimNN on Sat, Mar 18, 2:26 AM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=32130

The special case of zero sized values was previously not handled correctly. This patch handles this by not promoting if the size is zero.

Diff Detail

Repository
rL LLVM
TimNN created this revision.Sat, Mar 18, 2:26 AM
jmolloy requested changes to this revision.Sat, Mar 18, 2:35 AM

Hi Tim,

Thanks for looking into this. I'm not 100% certain about your patch though. The intent in this code is that everything gets padded to a multiple of 4 bytes (see PaddingPossible and RequiredPadding above). I'd expect that in the case you describe, RequiredPadding would be != 4, so the loop at line 3095 would padd out the global and avoid putting too-small data in the constant pool.

Without a reduced testcase (which would be needed to commit the change) I can't offhand see where the problem could actually be.

Cheers,

James

This revision now requires changes to proceed.Sat, Mar 18, 2:35 AM

Hi Tim,

I see, thanks. Yes, you're right, the abnormal case of Size == 0 would indeed blow this code up. Can I suggest:

  1. You add a "|| Size == 0" term to the if condition on line 3066
  2. You add a testcase.

The testcase should be a LIT test. You can see examples in the test/CodeGen/ARM directory which should be where this test should go. Have a look at test/CodeGen/ARM/constantpool-promote.ll - you can tag your test onto the end there.

Something like (appending after the @test9 function):

@zerosize = private unnamed_addr constant [0 x i16] zeroinitializer, align 4
;; Comment for this test
; CHECK-LABEL: @pr32130
; CHECK: -NOT: adr
define void @pr32130() #0 {
  tail call void @c(i16* getelementptr inbounds ([0 x i16], [0 x i16]* @zerosize, i32 0, i32 0)) #2
  ret void
}
TimNN updated this revision to Diff 92250.Sat, Mar 18, 4:35 AM
TimNN retitled this revision from arm: don't promote too small constants to [ARM] handle promotion of zero sized constants..
TimNN edited the summary of this revision. (Show Details)

I have updated the patch as suggested.

jmolloy accepted this revision.Sat, Mar 18, 4:52 AM

LGTM, thanks!

This revision is now accepted and ready to land.Sat, Mar 18, 4:52 AM
TimNN added a comment.Sat, Mar 18, 5:12 AM

I cannot commit this myself, could someone please do it for me?

This revision was automatically updated to reflect the committed changes.