This is an archive of the discontinued LLVM Phabricator instance.

Distinguish between code pointer size and DataLayout::getPointerSize() in DWARF info generation
ClosedPublic

Authored by kzhuravl on Mar 12 2017, 11:21 PM.

Diff Detail

Event Timeline

kzhuravl created this revision.Mar 12 2017, 11:21 PM

Reincarnation of D11969

echristo edited edge metadata.Mar 16 2017, 3:50 PM

In general, I'd prefer not to use the TargetMachine for this. This is already the only point where we use the DataLayout in the TargetMachine and I'd much rather use the values in the module for getting pointer sizes.

Thoughts?

mehdi_amini edited edge metadata.Mar 16 2017, 3:52 PM

Note that the existing code was unsigned AsmPrinter::getPointerSize() const { return TM.getPointerSize(); }, so it is not introducing necessarily new uses of the TM here.

mehdi_amini added inline comments.Mar 16 2017, 3:55 PM
include/llvm/CodeGen/AsmPrinter.h
202 ↗(On Diff #91516)

I think if you leave out this change, the patch will be *a lot* smaller and easier to review. I'm not against considering this change as a separate patch.

kzhuravl updated this revision to Diff 93272.Mar 28 2017, 12:35 PM
kzhuravl marked an inline comment as done.

Address review feedback.

mehdi_amini added inline comments.Apr 13 2017, 12:52 AM
test/DebugInfo/AMDGPU/code-pointer-size.ll
1

Are all these parameters useful?
why are you using -verify-machineinstrs?

16

Can you document what this test is checking?

test/DebugInfo/AMDGPU/variable-locations.ll
31

These were buggy before?

kzhuravl updated this revision to Diff 95333.Apr 14 2017, 12:32 PM
kzhuravl marked an inline comment as done.

Address review feedback.

test/DebugInfo/AMDGPU/code-pointer-size.ll
1

-verify-machineinstrs can be omitted since we are checking dwarf. Thanks.

test/DebugInfo/AMDGPU/variable-locations.ll
31

Yes.

kzhuravl added inline comments.Apr 14 2017, 12:44 PM
test/DebugInfo/AMDGPU/variable-locations.ll
31

This patch also fixes AMDGPU's code pointer size (see AMDGPUMCAsmInfo.cpp), resulting in this test change. Should AMDGPU's fix remain in this patch or be moved to a separate patch?

This revision is now accepted and ready to land.Apr 15 2017, 11:59 PM