This is an archive of the discontinued LLVM Phabricator instance.

[ARM] handle promotion of zero sized constants.

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




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

Event Timeline

TimNN created this revision.Mar 18 2017, 2:26 AM
jmolloy requested changes to this revision.Mar 18 2017, 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.



This revision now requires changes to proceed.Mar 18 2017, 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.Mar 18 2017, 4:35 AM
TimNN edited edge metadata.
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.Mar 18 2017, 4:52 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 18 2017, 4:52 AM
TimNN added a comment.Mar 18 2017, 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.